-
Notifications
You must be signed in to change notification settings - Fork 17
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: Add recursion_mode ('immediate' or 'deferred') to Signal and SignalInstance #293
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #293 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 1933 1973 +40
=========================================
+ Hits 1933 1973 +40 ☔ View full report in Codecov by Sentry. |
CodSpeed Performance ReportMerging #293 will improve performances by 10.36%Comparing Summary
Benchmarks breakdown
|
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.
I prefer to use flag, instead of a boolean variable, as it will allow extending in future.
I would like to see in this PR update of EventedModel to use dereferred mode by default.
I thinks that it will be nice to document somewhere why dereferred mode is better in some cases.
Co-authored-by: Grzegorz Bokota <[email protected]>
Co-authored-by: Grzegorz Bokota <[email protected]>
The failing benchmarks represent a slight increase in the time to create a signal instance:
which is not something I'm worried about here given that it's on the order of ns. @Czaki, this is ready for final review. I'll wait for your review on this one before merging. And I intend to follow up in another PR reviewing the exception handling logic slightly |
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.
In general, looks ok, with one remark.
fixes #292, and represents a partial reversion of #281 (the behavior of #281 is still possible, but must be opted into using recursion_mode = 'deferred')
This adds the possibility to control the order callbacks during recursive emission events (when one of the callbacks itself emits the same event):
"immediate"
: Nested emission events immediately begin a deeper emission loop:calling all callbacks with the arguments of the nested emitter before
returning to continue the original emission loop. This is the default
behavior.
"deferred"
: Nested emission events are deferred until the current emissionloop is complete. This is the default behavior.
@Czaki, thoughts welcome on parameter naming and signature, etc... (i.e. should it be a boolean
recurse_immediately
instead of a set of literal strings? will we ever want additional modes?)