Skip to content
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

Optimize Batch for empty IReadOnlyCollection<T> too #750

Merged
merged 2 commits into from
Aug 6, 2020

Conversation

leandromoh
Copy link
Collaborator

No description provided.

@leandromoh leandromoh requested a review from atifaziz July 24, 2020 15:49
Copy link
Member

@fsateler fsateler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should have tests for this optimization, but I don't think there was one for IReadOnlyList previously.

CI failure looks unrelated

@@ -100,7 +100,7 @@ public static IEnumerable<IEnumerable<TSource>> Batch<TSource>(this IEnumerable<
yield return resultSelector(bucket);
}
}
case IReadOnlyList<TSource> list when list.Count == 0:
case IReadOnlyCollection<TSource> collection when collection.Count == 0:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add tests for this

@atifaziz atifaziz added this to the 3.4.0 milestone Aug 6, 2020
@codecov
Copy link

codecov bot commented Aug 6, 2020

Codecov Report

Merging #750 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #750   +/-   ##
=======================================
  Coverage   93.03%   93.03%           
=======================================
  Files         106      106           
  Lines        3473     3473           
=======================================
  Hits         3231     3231           
  Misses        242      242           
Impacted Files Coverage Δ
MoreLinq/Batch.cs 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07bd086...e3cfba1. Read the comment docs.

@atifaziz atifaziz changed the title optimize Batch for IReadOnlyCollection<T> too Optimize Batch for IReadOnlyCollection<T> too Aug 6, 2020
@atifaziz atifaziz changed the title Optimize Batch for IReadOnlyCollection<T> too Optimize Batch for empty IReadOnlyCollection<T> too Aug 6, 2020
Copy link
Member

@atifaziz atifaziz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @fsateler's comment that a test should have been added.

but I don't think there was one for IReadOnlyList previously.

@fsateler There was one on line 123:

[TestCase(SourceKind.Sequence)]
[TestCase(SourceKind.BreakingList)]
[TestCase(SourceKind.BreakingReadOnlyList)]
[TestCase(SourceKind.BreakingCollection)]
public void BatchEmptySource(SourceKind kind)
{
var batches = Enumerable.Empty<int>().ToSourceKind(kind).Batch(100);
Assert.That(batches, Is.Empty);
}

Anyway, it was super quick to add another case so I went ahead and did it with e3cfba1.

@leandromoh Thanks for this PR!

@fsateler Thanks for your review.

@leandromoh
Copy link
Collaborator Author

leandromoh commented Aug 6, 2020

thank you both!

@atifaziz Merging is showing blocked for me. May you merge?

@atifaziz atifaziz merged commit 0961301 into master Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants