-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Allow string values for FlagEvaluationDetails.reason
and FlagResolutionDetails.reason
#264
Conversation
…gResolutionDetails.reason` Signed-off-by: Matthew Keeler <mkeeler@launchdarkly.com>
7b86983
to
e38c4a1
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #264 +/- ##
=======================================
Coverage 93.89% 93.89%
=======================================
Files 16 16
Lines 442 442
=======================================
Hits 415 415
Misses 27 27
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job 🍻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a huge python expert but the change and tests look good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @keelerm84! Thank you for your contribution. Change LGTM, but I am lost on what the test is supposed to assert.
If it's just a type check then it should be already covered by mypy. Would any of the tests break if you reverted the type change?
Signed-off-by: Matthew Keeler <mkeeler@launchdarkly.com>
d23d9a2
to
d35a2dd
Compare
That's a good point. I've removed the useless test and will rely on mypy as suggested. |
This PR
Expands the types allowed for
reason
onFlagEvaluationDetails
andFlagResolutionDetails
to include bothReason
andstr
types.Related Issues
Fixes #262
Notes
Related to spec issue: open-feature/spec#236