feat(parser): count "the number of <type> {returned|put into a graveyard} this way"#5734
Conversation
Parse changes introduced by this PR · 2 card(s), 3 signature(s) (baseline: main
|
There was a problem hiding this comment.
Code Review
This pull request introduces parsing and runtime support for landing-zone 'this way' quantities (such as 'returned this way' and 'put into a graveyard this way') to support cards like Volcanic Eruption and Barrel Down Sokenzan, accompanied by extensive unit and integration tests. The feedback points out that the custom loop-based suffix matching in parse_filtered_landing_zone_this_way violates the repository's strict rule (R1) requiring nom-based combinators for parser dispatch, and recommends refactoring the logic to use idiomatic nom combinators instead of manual string matching.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
c826fb2 to
23b1bcb
Compare
|
@matthewevans |
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the landing-zone quantity behavior and regressions are solid, but the new parser dispatch must be expressed as composed nom combinators.
🔴 Blocker
crates/engine/src/parser/oracle_quantity.rs:2239-2282dispatches the suffix grammar by manually iterating an ordered string table. This is parsing dispatch, so it must use one composable nom production (shared optional relative clause plus analt()for the landing-zone phrases), rather than a loop over near-duplicate string alternatives. Keep the existing filter validation and runtime behavior while moving the grammar into nom.
🟡 Non-blocking
- The change is correctly limited to controller-agnostic, nontrivial filters; retaining the Builder’s Bane negative is important when refactoring the grammar.
✅ Clean
- The current parse artifact is exactly the intended two-card class: Volcanic Eruption and Barrel Down Sokenzan.
crates/engine/tests/integration/landing_zone_this_way_quantity.rs:89-197drives both destroy-to-graveyard and bounce-to-hand tracked-set resolution through the real ability chain, with discriminating count assertions.- Current-head GitHub checks are terminal green.
Recommendation: request changes — replace the manual suffix loop with a composed nom parser, preserve the existing class and negative coverage, then re-request review.
…ard} this way"
Barrel Down Sokenzan ("deals damage equal to twice the number of Mountains
returned this way") and Volcanic Eruption ("...the number of Mountains put into
a graveyard this way") parsed the producer sub-effect but dropped the damage
sibling as Unimplemented -- the "equal to X" amount parser
(parse_event_context_quantity) never reached the tracked-set "this way" helpers
for landing-zone-phrased verb tails.
Add parse_filtered_landing_zone_this_way (the destination-phrased sibling of
parse_destroyed_or_sacrificed_this_way_filter): "returned this way" / "put into
a graveyard this way" on a controller-agnostic, type-specific filter ->
QuantityRef::FilteredTrackedSetSize{filter, caused_by: None}. Wire it into the
"the number of" block, and broaden parse_event_context_quantity to accept a bare
tracked-set Ref (no-multiplier form). The Bounce/Destroy producers already
publish the chain tracked set; "twice" composes via existing QuantityExpr::Multiply.
No new engine variant, no runtime change. Reveal/tap/per-recipient/aggregate
phrasings are intentionally excluded (their producers don't publish a countable
set) and stay honestly Unimplemented.
CR 608.2c (this-way identity), CR 400.7j (effect finds objects it moved),
CR 701.8a (destroy -> graveyard).
23b1bcb to
6074d09
Compare
matthewevans
left a comment
There was a problem hiding this comment.
Approved — the landing-zone tracked-set parser is now complete, idiomatic, and covered end to end.
🔴 Blocker
- None.
🟡 Non-blocking
- None.
✅ Clean
- The composed nom tail accepts the controller-agnostic specific-filter class while retaining the generic-card and controller-bearing negative guards.
- The current parse artifact exactly matches the two intended cards: Volcanic Eruption and Barrel Down Sokenzan.
- Registered runtime tests drive both destroy-to-graveyard and return-to-hand chains and distinguish the tracked-set count if this parser path is reverted.
Recommendation: approve and enqueue.
Coverage: Barrel Down Sokenzan ("deals damage equal to twice the number of Mountains returned this way") and Volcanic Eruption ("…the number of Mountains put into a graveyard this way") parsed the producer sub-effect but dropped the damage sibling as
Unimplemented— the "equal to X" amount parser (parse_event_context_quantity) never reached the tracked-set "this way" helpers for landing-zone-phrased verb tails.Fix: Add
parse_filtered_landing_zone_this_way— the destination-phrased sibling ofparse_destroyed_or_sacrificed_this_way_filter— matching "returned this way" / "put into a graveyard this way" on a controller-agnostic, type-specific filter →QuantityRef::FilteredTrackedSetSize{ filter, caused_by: None }. Wire it into the "the number of" block, and broadenparse_event_context_quantityto accept a bare tracked-setRef(the no-multiplier form). TheBounce/Destroyproducers already publish the chain tracked set; "twice" composes via existingQuantityExpr::Multiply.No new engine variant, no runtime change (single file). Reveal/tap/per-recipient/aggregate phrasings are intentionally excluded (their producers don't publish a countable set) and stay honestly
Unimplemented— negative tests guard against false coverage. Verified whole-workspace green:clippy --all-targets+ full engine suite (2934 integration + 16496 lib, 0 failed), 5 parser AST + 3 runtime tests.CR 608.2c (this-way identity), CR 400.7j (effect finds objects it moved), CR 701.8a (destroy → graveyard).