-
Notifications
You must be signed in to change notification settings - Fork 304
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
Fuse iterators in InterleaveShortest #872
base: master
Are you sure you want to change the base?
Fuse iterators in InterleaveShortest #872
Conversation
This brings InterleaveShortest in line with Interleave. The documentation already (incorrectly) stated that InterleaveShortest is fused.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #872 +/- ##
==========================================
- Coverage 94.38% 94.28% -0.11%
==========================================
Files 48 48
Lines 6665 6665
==========================================
- Hits 6291 6284 -7
- Misses 374 381 +7 ☔ View full report in Codecov by Sentry. |
I started an investigation ("blame" in the web pages of our code is even more powerful than I thought (for each line, we can have navigate its history)): |
I'm fine with either unfused or fused, but I'd vote for uniform behavior. Not sure if we have a general guideline regarding fusedness, though. |
I agree. |
I'm gonna make an issue about fusedness to see opened issues and PRs related to it, clarify the situation (like make a guideline) and eventually make a TODO list about all this. |
In my book,
InterleaveShortest
should not produce more items thanInterleave
, but so far it did for unfused source iterators.This PR brings
InterleaveShortest
in line with Interleave. The documentation already (incorrectly) stated thatInterleaveShortest
is fused.Related to #533.