Add Ertai's Trickery - #3572
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces parser support for targeted spell conditions, specifically handling cases where no mana was spent to cast a spell or when a spell was kicked, alongside bridging the SourceAttachedToCreature predicate for bestow triggers. The review feedback recommends enhancing these new parsers to support plural variants (such as "those spells" and "were kicked") by utilizing a modular subject parser instead of inline tag sequences, and adding corresponding test coverage to ensure robust sibling coverage.
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.
| fn parse_no_mana_spent_to_cast_target_condition(input: &str) -> OracleResult<'_, AbilityCondition> { | ||
| let (rest, _) = ( | ||
| tag("no mana was spent to cast "), | ||
| alt((tag("it"), tag("that spell"), tag("this spell"), tag("them"))), | ||
| ) | ||
| .parse(input)?; |
There was a problem hiding this comment.
Applying the Sibling coverage (L2) lens from the Review Style Guide, we should support the plural demonstrative "those spells" for the "no mana spent to cast" condition. To avoid fragile matches and combinatorial explosion, decompose this compound phrase by reusing a modular parser for the subject (such as parse_subject) instead of defining inline tag sequences.
| fn parse_no_mana_spent_to_cast_target_condition(input: &str) -> OracleResult<'_, AbilityCondition> { | |
| let (rest, _) = ( | |
| tag("no mana was spent to cast "), | |
| alt((tag("it"), tag("that spell"), tag("this spell"), tag("them"))), | |
| ) | |
| .parse(input)?; | |
| fn parse_no_mana_spent_to_cast_target_condition(input: &str) -> OracleResult<'_, AbilityCondition> { | |
| let (rest, _) = ( | |
| tag("no mana was spent to cast "), | |
| parse_subject, | |
| ) | |
| .parse(input)?; |
References
- L2. Sibling coverage: If a parser arm or string was extended, check if plural/possessive/negated/etc. variants are covered. (link)
- Avoid verbatim string equality for parsing Oracle phrases as it bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers for constituent parts (e.g., subjects, conjunctions) and compose them using idiomatic combinator aggregates (like nested
altandtagsequences) to prevent combinatorial explosion and improve maintainability.
| fn parse_was_kicked_condition(input: &str) -> OracleResult<'_, AbilityCondition> { | ||
| let (rest, _) = ( | ||
| alt((tag("it"), tag("that spell"), tag("this spell"))), | ||
| tag(" was kicked"), | ||
| ) | ||
| .parse(input)?; | ||
| Ok((rest, AbilityCondition::additional_cost_paid_any())) | ||
| } |
There was a problem hiding this comment.
Leveraging the Sibling coverage (L2) lens from the Review Style Guide, the parser should support plural variants of the "was kicked" suffix. To prevent combinatorial explosion and improve maintainability, decompose this compound phrase by reusing a modular parser for the subject (such as parse_subject) and composing it with the kicked status parser.
| fn parse_was_kicked_condition(input: &str) -> OracleResult<'_, AbilityCondition> { | |
| let (rest, _) = ( | |
| alt((tag("it"), tag("that spell"), tag("this spell"))), | |
| tag(" was kicked"), | |
| ) | |
| .parse(input)?; | |
| Ok((rest, AbilityCondition::additional_cost_paid_any())) | |
| } | |
| fn parse_was_kicked_condition(input: &str) -> OracleResult<'_, AbilityCondition> { | |
| let (rest, _) = ( | |
| parse_subject, | |
| alt((tag(" was kicked"), tag(" were kicked"))), | |
| ) | |
| .parse(input)?; | |
| Ok((rest, AbilityCondition::additional_cost_paid_any())) | |
| } |
References
- L2. Sibling coverage: If a parser arm or string was extended, check if plural/possessive/negated/etc. variants are covered. (link)
- Avoid verbatim string equality for parsing Oracle phrases as it bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers for constituent parts (e.g., subjects, conjunctions) and compose them using idiomatic combinator aggregates (like nested
altandtagsequences) to prevent combinatorial explosion and improve maintainability.
| #[test] | ||
| fn parse_was_kicked_suffix_condition_on_counter() { | ||
| let (cond, text) = strip_suffix_conditional( | ||
| "Counter target spell if it was kicked", | ||
| &mut ParseContext::default(), | ||
| ); | ||
| assert_eq!(text, "Counter target spell"); | ||
| assert_eq!(cond, Some(AbilityCondition::additional_cost_paid_any())); | ||
| } |
There was a problem hiding this comment.
Add a test case to verify that the plural variant "if they were kicked" is successfully parsed and stripped from the suffix, ensuring the sibling coverage is robust and regression-tested.
#[test]
fn parse_was_kicked_suffix_condition_on_counter() {
let (cond, text) = strip_suffix_conditional(
"Counter target spell if it was kicked",
&mut ParseContext::default(),
);
assert_eq!(text, "Counter target spell");
assert_eq!(cond, Some(AbilityCondition::additional_cost_paid_any()));
let (cond, text) = strip_suffix_conditional(
"Counter target spells if they were kicked",
&mut ParseContext::default(),
);
assert_eq!(text, "Counter target spells");
assert_eq!(cond, Some(AbilityCondition::additional_cost_paid_any()));
}References
- L2. Sibling coverage: If a parser arm or string was extended, check if plural/possessive/negated/etc. variants are covered. (link)
matthewevans
left a comment
There was a problem hiding this comment.
Thanks for this — the parser design is genuinely good: correct seam (target-anaphoric "if it was kicked" belongs at the suffix-conditional layer, mirroring the merged Nix parser, not in parse_inner_condition), pure nom combinators with no string-method dispatch, and CR 702.33d/608.2c are accurate. The condition is also correctly wired (AdditionalCostPaid is consumed at game/effects/mod.rs:6121, so it's not inert). Two things block merge:
[HIGH] Rebase required — the branch is off a stale base and conflicts with current main. This PR was branched hundreds of commits behind origin/main, so its diff appears to add things that already landed independently (parse_no_mana_spent_to_cast_target_condition, the tests/integration/main.rs registration block, etc.). git cherry-pick of this head onto current main conflicts in conditions.rs. Please rebase onto current origin/main and drop the now-duplicate bundled content, leaving only your real delta: parse_was_kicked_condition (+ _text wrapper), its wiring into strip_suffix_conditional / parse_condition_text, and the Ertai test.
[MED] The integration test is parser-shape only — it doesn't prove the counter is conditional at runtime. ertai_trickery_counter_kicked.rs:13-28 asserts the parsed condition matches!(... AdditionalCostPaid{..}) but never resolves the spell. A shape assertion passes even if the condition is ignored at resolution. Please add a GameScenario/GameRunner test that casts Ertai's Trickery kicked (assert the target spell is countered) and un-kicked (assert the target resolves) — i.e. a test that would fail if the counter became unconditional.
Heads-up: PR #3573 (Unravel) collides with this one — both insert at the same anchors in strip_suffix_conditional / parse_condition_text and bundle the same already-merged content. After you both rebase, whichever lands second will need to re-resolve those shared insertion points. No action needed beyond the rebase; just expect sequential landing.
Re-request review once rebased with the runtime test and I'll take another look — the core parser work is solid.
Parse counter suffix
if it was kickedas AdditionalCostPaid (Ertai's Trickery).