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. |
toddbaert
left a comment
There was a problem hiding this comment.
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. |
matthewelwell
left a comment
There was a problem hiding this comment.
@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? |
matthewelwell
left a comment
There was a problem hiding this comment.
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. |
gruebel
left a comment
There was a problem hiding this comment.
great work, just few minor comments/questions
matthewelwell
left a comment
There was a problem hiding this comment.
Looks good, thanks @federicobond !
Signed-off-by: Federico Bond <federicobond@gmail.com>
Signed-off-by: Federico Bond <federicobond@gmail.com>
Signed-off-by: Federico Bond <federicobond@gmail.com>
Signed-off-by: Federico Bond <federicobond@gmail.com>
Signed-off-by: Federico Bond <federicobond@gmail.com>
Signed-off-by: Federico Bond <federicobond@gmail.com>
Signed-off-by: Federico Bond <federicobond@gmail.com>
…ssociated state Signed-off-by: Federico Bond <federicobond@gmail.com>
Signed-off-by: Federico Bond <federicobond@gmail.com>
…er_status Signed-off-by: Federico Bond <federicobond@gmail.com>
Signed-off-by: Federico Bond <federicobond@gmail.com>
Signed-off-by: Federico Bond <federicobond@gmail.com>
|
Merged! 🎉 Thanks everyone for your helpful comments and suggestions. |
|
Amazing, nice job @federicobond! |
This PR
Follow-up Tasks
Fixes: #278