Add onEach() and make forEach() return void on Collection and Map - #41
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
delacry
left a comment
There was a problem hiding this comment.
Map is deliberately untouched: forEach(), forEachKey() and forEachValue() still return the map. Collection and Map diverge here on purpose, and aligning them is a separate call.
I think it should also be aligned, maybe as a part of this PR, because $map->forEach() returns the map, but $map->values->forEach() returns void and you have always remember what you working with and it might be confusing (even for AI generating that code).. so I'm suggesting to rename existing forEach* (3) methods to onEach* and add 3 more forEach* methods which return void, what do you think?
btw, claude scan found some stale comments which are now false, it should be edited as part of this PR:
Stale comments that now say the wrong thing (should fix)
1. src/Sequence/Sequence.php:231: the forEach() docblock still says it is a "deliberate divergence from Collection::forEach(), which still hands the collection back". After this PR, both return void. The PR doesn't touch this file.
2. docs/collection/getting-started.md:143: the comment // returns itself for chaining above $set->forEach(...) is now wrong. The PR fixed the same comment in cheatsheet.md and missed this one.
Split the two uses of forEach() apart, as Sequence already does: onEach() walks the elements and hands the collection back for chaining, forEach() walks them and returns nothing. forEach() was declared by hand in eighteen interfaces, each with its own narrowed return type. Those declarations become onEach() unchanged, while forEach(): void is declared once on Collection: a void return matches no return-type pattern, so it leaves the narrowing generator entirely. The five Collection-side blacklists move from forEach to onEach; Map keeps its own. Regenerating leaves every generated block untouched. Map is deliberately left alone: forEach(), forEachKey() and forEachValue() still return the map. Update the API reference and the cheatsheet entry that promised a return value.
Collection and Sequence now share the same body: walk $this and hand each element to the action, returning nothing.
Align Map with Collection and Sequence: $map->forEach() returned the map while $map->values->forEach() returned nothing, so the same call meant a different thing depending on the receiver. forEach(), forEachKey() and forEachValue() become onEach(), onEachKey() and onEachValue() with their narrowed return types unchanged, and the forEach*() trio is declared once on Map with a void return, which keeps it out of the narrowing generator. The Map blacklists move from forEach* to onEach*.
9ef5fb4 to
58fb348
Compare
Follow-up from #23, where we agreed to split the two uses of
forEach()apart the waySequencealready does:onEach()walks the elements and hands the receiver back for chaining,forEach()walks them and returns nothing.Collection,MapandSequencenow agree:forEach*()ends the chain,onEach*()keeps it going. Before,$map->forEach()returned the map while$map->values->forEach()returned nothing, so the same call meant a different thing depending on the receiver.Why this costs almost nothing
forEach()was declared by hand in eighteen Collection interfaces, each just above the auto-generated narrowing block, each with its own narrowed return type —ImmutableList<E>,MutableTrackedSet<E>&TrackedResult, and so on. Those declarations becomeonEach()with the same type, one line each.forEach(): voidis then declared once, onCollection. Avoidreturn matches none of the generator's return-type patterns, so the method leaves the narrowing machinery entirely: no narrowed declaration left to maintain anywhere. The five Collection-side blacklists move fromforEachtoonEach— they do not multiply.Mapfollows the same path:forEach(),forEachKey()andforEachValue()becomeonEach(),onEachKey()andonEachValue()with their narrowed return types unchanged, and theforEach*()trio is declared once onMapwith avoidreturn. The Map blacklists move fromforEach*toonEach*. Regenerating afterwards only renames the generated declarations.The implementation does not duplicate the walk:
And the Collection-side
forEach()is now identical toSequence's, so it lives once inIterableTerminalsLogic.No
#[NoDiscard]ononEach()It looked like the obvious companion to the attribute
Sequence::onEach()carries, but it does not fit here. The generator copies attributes into the*Tracked*interfaces, which useNoDiscardnowhere and do not import it — sixattribute.notFounderrors. And the reason the sequence carries it is laziness: dropping a lazyonEach()means nothing ever runs. On an eager collection the action has already run by the time the return value is dropped, so the attribute would flag a style preference, not a bug.forEach()never carried it either.Scope
This is a hard break with no deprecation shim, which is what the 0.2.x branch is for. Code that chains off
forEach(),forEachKey()orforEachValue()now fails loudly rather than silently; the fix is a rename to the matchingonEach*().docs/collection/api/collection.mdanddocs/collection/api/map.mdgain the new entries,map.md's chaining example moves toonEach(), and the comments incheatsheet.md,getting-started.mdandSequence::forEach()that still promised a return value are fixed.