Repository navigation
Make the aggregate vocabulary and its column rule introspectable - #22
Merged
Merged
Conversation
Two facts about aggregates lived only inside their implementations. Which
aggregates exist was a closed match in AggregateFactory::for(), readable
but not enumerable. Which ones need an aggregateColumn was stated four
times, once per implementation, as a literal in the message it throws
when the column is missing:
throw new RuntimeException("aggregateColumn is required when using 'sum' aggregate");
Nothing in the Aggregate interface said the rule existed. A caller wanting
to validate a lookup before running it, or to ask for a column only where
one means something, had to write both lists out again and keep them in
step with this package by hand.
AggregateKind is now the vocabulary — the one place the list is written —
and answers both questions without an aggregate in hand:
AggregateKind::names(); // ['first', 'last', 'count', 'sum', ...]
AggregateKind::Sum->requiresColumn(); // true
AggregateKind::Count->requiresColumn(); // false
The requirement sits on the kind rather than the instance because the
caller asking it has a name, not a state to fold records into; an
aggregate reaches the same answers through kind(), now on the interface.
The four column-requiring aggregates share one guard that reads its name
off its own kind, so the requirement the enum advertises and the refusal
an aggregate makes cannot drift apart — and a test walks every case
asserting the two agree, so a kind added without a verdict fails there.
AggregateFactory::for(string) is unchanged for callers: it stays the door
from a persisted name to a starting state, refusing unknown names with
the same message, and now delegates the list to the enum.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.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.
Two facts about aggregates lived only inside their implementations.
Which aggregates exist was a closed
matchinAggregateFactory::for()— readable, but not enumerable.Which ones need an
aggregateColumnwas stated four times, once per implementation, as a literal in the message it throws when the column is missing:Nothing in the
Aggregateinterface said the rule existed. A caller wanting to validate a lookup before running it, or to ask for a column only where one means something, had to write both lists out again and keep them in step with this package by hand.AggregateKindis now the vocabularyOne place the list is written, answering both questions without an aggregate in hand:
The requirement sits on the kind rather than the instance because the caller asking has a name, not a state to fold records into —
AggregateFactory::for('sum')->requiresColumn()works but reads oddly for a question about the kind. An aggregate reaches the same answers throughkind(), now on the interface.The four column-requiring aggregates share one guard (
RequiresAggregateColumn) that reads its name off its own kind, so no aggregate spells its own name and the requirement the enum advertises cannot drift from the refusal an aggregate makes. A census test walks every case asserting the two agree — a kind added without a verdict fails there:Compatibility
AggregateFactory::for(string)is unchanged for callers: still the door from a persisted name to a starting state, still refusing unknown names withUnknown aggregate: x, now delegating the list to the enum. No BC break —LookupSource::$aggregatestays a string.Not in scope
DatabaseLookupResolver(on another branch) restates the same vocabulary in two morematches and has its own copy of the column rule. It can now read both fromAggregateKind; left alone here to keep this change to one concern.Verification
151 tests, 100% line coverage, 100% MSI, PHPStan clean, Pint clean — all four on PHP 8.4.
Two coverage-metadata adjustments were needed and are worth flagging for review:
LookupSourceTestandExecutionObserverTestgained#[UsesClass(AggregateKind::class)], because a new class on the resolver's path makes every test that walks it risky underbeStrictAboutCoverageMetadata; and the vocabulary assertion lives inAggregateTest, which covers the enum, rather than inAggregateFactoryTest, which only uses it — otherwisenames()reads as uncovered.The README gains an Aggregates section documenting both facts.
🤖 Generated with Claude Code