morelinq / MoreLINQ

Extensions to LINQ to Objects
https://morelinq.github.io/
Apache License 2.0
3.7k stars 415 forks source link

Fix collection-optimized paths for `Batch` #965

Closed viceroypenguin closed 1 year ago

viceroypenguin commented 1 year ago

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.

viceroypenguin commented 1 year ago
  1. Oops. Wilco.
  2. Will restore for now, and issue separate PR to remove them after this PR is pulled in. Discussion to be tabled until then.
codecov[bot] commented 1 year ago

Codecov Report

Merging #965 (5057527) into master (aefa90b) will increase coverage by 0.05%. The diff coverage is 100.00%.

:exclamation: Current head 5057527 differs from pull request most recent head 2d49f5a. Consider uploading reports for the commit 2d49f5a to get more accurate results

@@            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     
Impacted Files Coverage Δ
MoreLinq/Batch.cs 98.07% <100.00%> (+3.73%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

atifaziz commented 1 year ago

and issue separate PR to remove them after this PR is pulled in

What's the reason behind removing them at all?

viceroypenguin commented 1 year ago

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. :)

atifaziz commented 1 year ago

@viceroypenguin Any chance we can merge this soon?