-
Notifications
You must be signed in to change notification settings - Fork 50
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 async transaction callbacks #2708
feat: Add async transaction callbacks #2708
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2708 +/- ##
========================================
Coverage 77.87% 77.88%
========================================
Files 310 310
Lines 23125 23120 -5
========================================
- Hits 18008 18005 -3
- Misses 3728 3729 +1
+ Partials 1389 1386 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
for _, fn := range fns { | ||
fn() | ||
} | ||
for _, fn := range asyncFns { |
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.
todo: The async functions need to be called before the non-async one otherwise they are less async than they could be..
for _, fn := range t.discardFns { | ||
fn() | ||
} | ||
for _, fn := range t.discardAsyncFns { |
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.
todo: Same here. Asyncs need to be called first.
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.
LGTM. Please resolve the two minor todos before merging :)
Relevant issue(s)
Resolves #2707
Description
This PR adds async versions for all transaction callbacks.
Tasks
How has this been tested?
make test
Specify the platform(s) on which this was tested: