[Repo Assist] refactor: simplify lengthBy and lengthBeforeMax using while!#352
Draft
github-actions[bot] wants to merge 2 commits intomainfrom
Draft
Conversation
Replace the pattern of: - one unconditional MoveNextAsync call before the loop - a mutable 'go' flag - manual go/step tracking ...with the simpler 'while! e.MoveNextAsync() do' form. For lengthBy (all three predicate variants), the new implementation is cleaner and reads more naturally: iterate over elements, count those that match. For lengthBeforeMax, the new implementation advances the enumerator inside the loop body and guards with 'go && i < max', eliminating the pre-loop advance. A beneficial side effect: the old implementation had a read-ahead of 1 (an extra MoveNextAsync call before the loop), which meant lengthOrMax 5 on a 10-element source would evaluate 6 elements instead of 5, and lengthOrMax 0 would still evaluate the first element. The new implementation has no read-ahead, so lengthOrMax N evaluates exactly N elements as expected. Tests updated to reflect the improved (no read-ahead) behaviour, replacing the N+1 and 1 assertions with the correct N and 0. All 5013 existing tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 This is an automated pull request from Repo Assist.
Summary
Simplify
lengthByandlengthBeforeMaxby replacing the pre-loopMoveNextAsync+ mutablegopattern with idiomaticwhile!loops.Before
After
The
while! e.MoveNextAsync() doidiom is the standard pattern used throughout the rest of this codebase (e.g.maxMin,sum,average,contains). Applying it here bringslengthByin line with those functions and removes the confusing "count before moving" comment.Improved semantics for
lengthBeforeMaxThe old implementation had an accidental "read-ahead by 1": it called
MoveNextAsynconce unconditionally before the loop, meaningTaskSeq.lengthOrMax 5on a longer sequence would evaluate 6 elements instead of 5, andTaskSeq.lengthOrMax 0would evaluate 1 element instead of 0.The new implementation is cleaner and evaluates exactly
min(length, max)elements — no read-ahead:Three tests in
TaskSeq.Length.Tests.fsthat explicitly asserted the old N+1 / 1 read-ahead counts have been updated to assert the correct N / 0 values, with revised test names and comments.Test Status
Build: ✅ succeeded (0 warnings, 0 errors)
Tests: ✅ 5013 passed, 2 skipped (infrastructure), 0 failed
Branch:
repo-assist/improve-length-clarity-20260317