-
Notifications
You must be signed in to change notification settings - Fork 30
fix: Allow string values for FlagEvaluationDetails.reason and FlagResolutionDetails.reason
#264
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
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. |
gruebel
left a comment
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 🍻
lukas-reining
left a comment
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!
federicobond
left a comment
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. |
Signed-off-by: OpenFeature Bot <109696520+openfeaturebot@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This PR
Expands the types allowed for
reasononFlagEvaluationDetailsandFlagResolutionDetailsto include bothReasonandstrtypes.Related Issues
Fixes #262
Notes
Related to spec issue: open-feature/spec#236