-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Misc fixes for summary callback changes #1788
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1788 +/- ##
==========================================
+ Coverage 71.43% 71.60% +0.17%
==========================================
Files 178 178
Lines 13777 13789 +12
==========================================
+ Hits 9841 9874 +33
+ Misses 3323 3303 -20
+ Partials 613 612 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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, just a minor point that we might be degrading the marshalling performance, but someone will have to run/write a benchmark that uses it to see how much it is degrading if at all
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 fixes #1316 and contains the prerequisite changes (and a few minor unrelated ones) for #1768. I've taken the extraneous changes from that PR, split them apart in sensible commits, and added some more tests. Hopefully, the only thing that remains in #1768 would be the actual
handleSummary()
changes, though I may have a few other minor commits to make here before I push the final version of the other PR (likely tomorrow).