-
Notifications
You must be signed in to change notification settings - Fork 414
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
Fix collection-optimized paths for Batch
#965
Conversation
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.
Good catch and thanks for submitting the PR!
Two things:
- There are failing tests that need to be adjusted.
- The PR removes all optimisations whereas they should have been moved into the iterator method for similar savings at iteration-time.
|
Codecov Report
@@ Coverage Diff @@
## master #965 +/- ##
==========================================
+ Coverage 92.49% 92.55% +0.05%
==========================================
Files 113 113
Lines 3424 3423 -1
Branches 1024 1020 -4
==========================================
+ Hits 3167 3168 +1
Misses 192 192
+ Partials 65 63 -2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
What's the reason behind removing them 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.
Thanks for the follow-up, but I am afraid the test and the implementation don't look right. See my comments.
Bah! Too many branches on too many projects. :) |
Co-authored-by: Atif Aziz <[email protected]>
MoreLinq.Test/BatchTest.cs
Outdated
[TestCase(SourceKind.BreakingList)] | ||
[TestCase(SourceKind.BreakingReadOnlyList)] | ||
[TestCase(SourceKind.BreakingCollection)] | ||
public void BatchCollectionSmallerThanSizeUsesCollectionCountAtIterationTime(SourceKind kind) |
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.
The two could be combined into a effectively single test?
[TestCase(SourceKind.BreakingList)]
[TestCase(SourceKind.BreakingReadOnlyList)]
[TestCase(SourceKind.BreakingCollection)]
public void BatchUsesCollectionCountAtIterationTime(SourceKind kind)
{
var list = new List<int> { 1, 2 };
var result = list.ToSourceKind(kind, copy: false).Batch(3);
list.Add(3);
Assert.That(result.Consume, Throws.Nothing);
list.Add(4);
Assert.That(result.Consume, Throws.TypeOf<BreakException>());
}
@viceroypenguin Any chance we can merge this soon? |
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 this!
The
Batch
operator contains similar problems to those addressed in issue #943/PR #946. This PR removes all collection optimizations, since they are all based on caching the collection count at initial call instead of run-time.