-
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
feat: implement provider status #288
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #288 +/- ##
==========================================
- Coverage 94.71% 94.06% -0.66%
==========================================
Files 18 18
Lines 492 539 +47
==========================================
+ Hits 466 507 +41
- Misses 26 32 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
69119b6
to
9add52e
Compare
Signed-off-by: Federico Bond <federicobond@gmail.com>
9add52e
to
ac530f8
Compare
The "short circuiting" behavior mentioned in 1.7.6 and 1.7.7 might be relevant enough to implement here as well: https://github.com/open-feature/spec/blob/37cf68b0d68b6814514bcded521b9e199efcead3/specification/sections/01-flag-evaluation.md?plain=1#L485-L502 |
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.
Nothing looks wrong here to me, but there's a few related things that might be worth adding.
…L error Signed-off-by: Federico Bond <federicobond@gmail.com>
Signed-off-by: Federico Bond <federicobond@gmail.com>
Signed-off-by: Federico Bond <federicobond@gmail.com>
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.
This looks fully spec compliant to me, as far as provider status stuff goes.
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 work 🏅
This self-contained change introduces the functionality required to properly implement provider status events later. It has no user-visible changes at this point.
I wanted to split this out of the events pull request because it had the potential to snowball the size of the diff and lower the quality of the review in general.
I've introduced several seams that will make it easy to attach event handlers that respond to status changes later on.