Repository navigation
[Sequence] Add "terminal" methods: aggregation - #32
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Third of four PRs bringing the terminal vocabulary of decision 5 to Sequence: fold, reduce/reduceOrNull, sum, avg/avgOrNull, min/max (+ OrNull), minOf/maxOf (+ OrNull) and joinToString. Every body in this batch walks $this once and reads nothing else, so all of them move out of CollectionLogic into the shared IterableTerminalsLogic rather than being copied into SequenceLogic - the line noctud#29 drew and noctud#31 followed. SequenceLogic gains nothing at all: both sides already use the trait, so the sequence picks these up by declaring them on the interface. Collection behaviour is unchanged, and its existing suite is what proves it. The OrNull variants stop being try-catch wrappers, which is the one behavioural change here. reduceOrNull() around reduce() swallowed an UnsupportedOperationException raised by the caller's own operation, and minOrNull()/maxOrNull()/minOfOrNull()/maxOfOrNull() swallowed a NoSuchElementException raised by the caller's selector - answering "empty" about a subject that was not empty. Each now walks on its own and returns null when nothing was found; two parity tests on the eager side pin the propagation. The empty-subject messages move to named constructors on UnsupportedOperationException (cannotReduceEmptySubject, cannotAverageEmptySubject) next to NoSuchElementException::emptySubject, so a sequence is told a "sequence" is empty. lcfirst leaves avg()'s eager wording byte-identical; reduce() gains the message it never carried. None of these take #[NoDiscard]: they hand back a scalar or an element rather than a new container, which is where noctud#29 put the attribute. Claude-Session: https://claude.ai/code/session_01JCXbhGog8jh6JAeYSCjy9y
nikophil
force-pushed
the
feature/sequence-aggregation
branch
from
August 24, 2026 11:16
31f8ca7 to
46f3c96
Compare
nikophil
marked this pull request as ready for review
August 24, 2026 11:26
Member
|
Hi, sorry for the late response, I was on a vacation, and then I needed another vacation from my vacation 😄 so I'll try to look at it by the end of this week. |
Contributor
Author
|
Haha no rush, hope vacation were good |
delacry
requested changes
Sep 12, 2026
joinToString() now appends to a string as it walks instead of collecting every part and imploding at the end: on a sequence over a large file the parts array was a second copy of the whole input. Its docblock also says what the limit really costs - $limit + 1 pulls, the extra element being what decides whether $truncated applies. Tests: the OrNull extremes propagation test covers all four methods, the drain test covers every aggregation rather than three, and the reduceOrNull propagation parity test lives with the other reduce tests.
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.
Third of four PRs bringing the terminal vocabulary to
Sequence— element accesslanded in #29, querying in #31, aggregation here, then
forEach/toMap. This addsfold,reduce/reduceOrNull,sum,avg/avgOrNull,min/max(+OrNull),minOf/maxOf(+OrNull) andjoinToString.The whole batch is shared
#29 settled that a body identical on both sides lives in
IterableTerminalsLogicrather than being copied, and #31 split five methods off that way. This time
nothing splits: 232 lines leave
CollectionLogicfor the trait andSequenceLogicgains not a single line — both sides alreadyusethe trait, sothe sequence picks these up by declaring them on the interface.
That is not a coincidence. What kept
isEmpty,containsandcountper side in#31 was the store: the eager side answers them in O(1) where a sequence has to
pull. An aggregation has no such shortcut —
sum,min,avgand friends have tovisit every element whatever they are backed by, so the eager body is the lazy
body.
Collectionbehaviour is unchanged, and its existing suite is what provesit.
The
OrNullvariants stop swallowing the caller's exceptionsThis is the one behavioural change, and it touches
Collectiontoo. The eagerOrNullmethods were try-catch wrappers:The
catchcannot tell its own empty-subject throw from one raised inside$operation, which is user code. SoreduceOrNull()reportednull— "thesubject was empty" — for a subject that was not empty at all, and the same held for
minOrNull/maxOrNull/minOfOrNull/maxOfOrNullagainst a selector raisingNoSuchElementException. Rare, but it turns a real error into a plausible answer,which is the failure mode worth spending code on.
Each
OrNullnow walks on its own and returnsnullwhen nothing was found — noflag needed, the accumulator simply stays
null. Two parity tests on the eagerside pin the propagation
(
reduceOrNull_propagates_an_UnsupportedOperationException_thrown_by_the_operation,minOrNull_propagates_a_NoSuchElementException_thrown_by_the_selector), andthe_OrNull_extremes_propagate_a_NoSuchElementException_thrown_by_the_selectordoes the same on the lazy side. The duplication that buys is four short loops.
Empty-subject messages
UnsupportedOperationExceptiongetscannotReduceEmptySubject()andcannotAverageEmptySubject(), joiningNoSuchElementException::emptySubject()onthe
NamesItsSubjecttrait — a sequence must not be told a "Collection" is empty.lcfirstleavesavg()'s eager wording byte-identical;reduce()gains themessage it never carried (it threw a bare
UnsupportedOperationException).Laziness, pinned
Aggregations drain — that is what they are — but the tests say so out loud rather
than leaving it implied:
the_other_aggregations_drain_the_sourcecounts what thesource handed out,
an_aggregation_consumes_a_pass_of_a_one_shot_sourceandaggregation_replays_over_a_replayable_sourcehold the replayability contract, andjoinToString_stops_pulling_at_the_limitshows the one method here thatshort-circuits: past
$limitit appends$truncatedand stops asking.aggregation_matches_its_collection_counterpartasserts the eager and lazy answersare the same values.
Types
tests/Type/Sequence/AggregationTypeTest.phpcovers what the PHPDoc promises:foldreturns the accumulator typeRrather thanE,min/maxhand backE(declared
: mixed) whileminOf/maxOfhand back the selector'sR, andsum'sconditional return type narrows to
intfor a sequence of ints and widens tofloat|intas soon as a float appears on either side — including when it appearsthrough a
map()upstream rather than at the source.#[NoDiscard]None of these take it — they hand back a scalar or an element rather than a new
container, which is where #29 settled the rule.
Not in this batch
toMap()andforEach()are the fourth PR.findLast/expectLastandrandom/randomOrNullare still open questions on #23 rather than omissions.Related to #23, follows #31.