-
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 events #278
Conversation
641102d
to
caff259
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #278 +/- ##
==========================================
+ Coverage 94.19% 94.77% +0.58%
==========================================
Files 18 20 +2
Lines 551 651 +100
==========================================
+ Hits 519 617 +98
- Misses 32 34 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
I'll take a close look at this in the next couple days. |
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.
Please pardon my lack of Python skills if I'm missed something.
Overall the structures and API look great!
I left some requested changes specifically on the finer points of the eventing spec. They can be tricky, and some of it could be better fleshed-out in the spec, as I think you noted before. I will work on some additional supporting explanations today. Thanks for your efforts here! 🙏
@federicobond I've opened open-feature/spec#248 |
8cb80f5
to
17b3369
Compare
This should be ready to review from a specification perspective, we might want to add some further tests maybe. |
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.
@federicobond I've added a few comments here from a python perspective but I don't purport to be an expert so feel free to ignore as you see fit!
From Requirement 5.1.1:
Should we add a guard to make sure this cannot happen? |
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.
Apologies for the delay in responding here @federicobond, I really need to look into my GH notifications 🙄
I've added a few more comments in response to yours. Again, I'm happy to be ignored on any of them if you see fit.
@matthewelwell I refactored the EventSupport implementation so that we are no longer using private module imports. It should be much cleaner now. |
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.
great work, just few minor comments/questions
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.
Looks good, thanks @federicobond !
Signed-off-by: Federico Bond <[email protected]>
Signed-off-by: Federico Bond <[email protected]>
Signed-off-by: Federico Bond <[email protected]>
Signed-off-by: Federico Bond <[email protected]>
Signed-off-by: Federico Bond <[email protected]>
Signed-off-by: Federico Bond <[email protected]>
Signed-off-by: Federico Bond <[email protected]>
…ssociated state Signed-off-by: Federico Bond <[email protected]>
Signed-off-by: Federico Bond <[email protected]>
…er_status Signed-off-by: Federico Bond <[email protected]>
Signed-off-by: Federico Bond <[email protected]>
Signed-off-by: Federico Bond <[email protected]>
Merged! 🎉 Thanks everyone for your helpful comments and suggestions. |
Amazing, nice job @federicobond! |
This PR
Follow-up Tasks
Fixes: #278