-
Notifications
You must be signed in to change notification settings - Fork 30
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
(Towards 2421) Suggested fix to inline trans #2430
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2430 +/- ##
==========================================
- Coverage 99.84% 99.84% -0.01%
==========================================
Files 350 350
Lines 47265 47272 +7
==========================================
+ Hits 47194 47200 +6
- Misses 71 72 +1 ☔ View full report in Codecov by Sentry. |
@arporter This is my proposed fix for the issues I had with inlining, which should enable more general usage for parallel loops. |
For some reason, github isn't letting me complete my review properly. The fix looks fine. I think it just needs some comments and an explicit (new) test of the bug that is being fixed. I'll trigger the integration tests now. Note that I've merged master into the branch so you'll need to |
@arporter I added a test to explicitly check this and some comments - let me know if you feel they're enough or is there's anything I've missed. |
I'm confused as to why this reports a loss in coverage though? Nothing I've changed should touch the |
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.
Thanks for those changes. All looks good now and integration tests were fine.
Will proceed to merge.
No description provided.