diff --git a/docs/prs/pr-011-resolve-with-args.md b/docs/prs/pr-011-resolve-with-args.md new file mode 100644 index 00000000..f3ba35bf --- /dev/null +++ b/docs/prs/pr-011-resolve-with-args.md @@ -0,0 +1,44 @@ +# PR 11: Extract reflection-based resolve! argument passing + +## Summary + +Four `resolve!` implementations in the action layer all used identical reflection logic to build a keyword-argument hash from a receiver's method signature before calling `resolve!`. A signature change in any card or ability's `resolve!` would silently pass wrong arguments at all four sites. This PR extracts the logic into a single `ResolvesWithArgs` module. + +## Why + +The docs note in improvement #4: + +> Using `.method().parameters` to decide what arguments to pass is fragile — a signature change silently breaks the call — and the duplication means fixes need applying twice. + +With the helper centralised, any fix or additional argument (e.g. a future `mode:` or `zone:` keyword) is added in one place. + +## What Changed + +**`lib/magic/resolves_with_args.rb`** (new file) +- Defines `Magic::ResolvesWithArgs` with a single private method `resolve_with_args(receiver, **available)` +- Introspects `receiver.method(:resolve!).parameters` and filters `available` to only the keys the method declares (both `:keyreq` and `:key` parameter kinds) +- Calls the method with the filtered args + +**`lib/magic/action.rb`** +- `include ResolvesWithArgs` — covers `Cast`, `ActivateAbility`, `ActivateLoyaltyAbility` + +**`lib/magic/actions/cast/mode.rb`** +- `include Magic::ResolvesWithArgs` — `Mode` does not inherit from `Action` so it includes the module directly + +**`lib/magic/actions/cast.rb`** +- `resolve!` replaced 6-line reflection block with `resolve_with_args(card, target:, targets:, kicked:, value_for_x:)` + +**`lib/magic/actions/activate_ability.rb`** +- `resolve!` replaced 4-line reflection block with `resolve_with_args(ability, target:, targets:)` + +**`lib/magic/actions/activate_loyalty_ability.rb`** +- `resolve!` replaced 5-line reflection block with a 2-line form; `value_for_x` is added to the pool only when non-nil (preserving the original guard) + +**`lib/magic/actions/cast/mode.rb`** +- `resolve!` replaced 4-line reflection block with `resolve_with_args(mode, target:, targets:)` + +## Invariants Preserved + +- The filtering logic is identical to the original: a keyword arg is passed only if the receiver's `resolve!` declares it (required or optional) +- The `value_for_x` nil guard in `ActivateLoyaltyAbility` is preserved — X is excluded from the pool when it was not set, so no ability receives `value_for_x: nil` unexpectedly +- All 534 tests pass diff --git a/lib/magic/action.rb b/lib/magic/action.rb index 36b6475e..a636ca97 100644 --- a/lib/magic/action.rb +++ b/lib/magic/action.rb @@ -1,5 +1,7 @@ module Magic class Action + include ResolvesWithArgs + attr_reader :game, :player def initialize(game:, player:) diff --git a/lib/magic/actions/activate_ability.rb b/lib/magic/actions/activate_ability.rb index e3273ee5..a2ee9569 100644 --- a/lib/magic/actions/activate_ability.rb +++ b/lib/magic/actions/activate_ability.rb @@ -112,11 +112,7 @@ def finalize_costs!(player) end def resolve! - resolver = ability.method(:resolve!) - args = {} - args[:target] = targets.first if resolver.parameters.include?([:keyreq, :target]) - args[:targets] = targets if resolver.parameters.include?([:keyreq, :targets]) - ability.resolve!(**args) + resolve_with_args(ability, target: targets.first, targets: targets) end end end diff --git a/lib/magic/actions/activate_loyalty_ability.rb b/lib/magic/actions/activate_loyalty_ability.rb index 3d930108..d8758303 100644 --- a/lib/magic/actions/activate_loyalty_ability.rb +++ b/lib/magic/actions/activate_loyalty_ability.rb @@ -44,12 +44,9 @@ def perform end def resolve! - resolver = ability.method(:resolve!) - args = {} - args[:target] = targets.first if resolver.parameters.include?([:keyreq, :target]) - args[:targets] = targets if resolver.parameters.include?([:keyreq, :targets]) - args[:value_for_x] = x_value if x_value && resolver.parameters.include?([:keyreq, :value_for_x]) - ability.resolve!(**args) + pool = { target: targets.first, targets: targets } + pool[:value_for_x] = x_value if x_value + resolve_with_args(ability, **pool) end end end diff --git a/lib/magic/actions/cast.rb b/lib/magic/actions/cast.rb index 48ced7d0..816b8dbc 100644 --- a/lib/magic/actions/cast.rb +++ b/lib/magic/actions/cast.rb @@ -142,18 +142,14 @@ def choose_mode(mode_class, &) def resolve! if modes.any? - modes.each do |mode| - mode.resolve! - end + modes.each { |mode| mode.resolve! } else - resolver = card.method(:resolve!) - args = {} - args[:target] = targets.first if resolver.parameters.include?([:keyreq, :target]) - args[:targets] = targets if resolver.parameters.include?([:keyreq, :targets]) - args[:kicked] = kicker_cost.paid? if resolver.parameters.include?([:key, :kicked]) - args[:value_for_x] = mana_cost.x if resolver.parameters.include?([:keyreq, :value_for_x]) - - card.resolve!(**args) + resolve_with_args(card, + target: targets.first, + targets: targets, + kicked: kicker_cost.paid?, + value_for_x: mana_cost.x, + ) end if card.sorcery? || card.instant? diff --git a/lib/magic/actions/cast/mode.rb b/lib/magic/actions/cast/mode.rb index 219eac1a..17218209 100644 --- a/lib/magic/actions/cast/mode.rb +++ b/lib/magic/actions/cast/mode.rb @@ -2,6 +2,8 @@ module Magic module Actions class Cast < Action class Mode + include Magic::ResolvesWithArgs + attr_reader :mode, :targets def initialize(mode) @@ -35,12 +37,7 @@ def targeting(*targets) end def resolve! - resolver = mode.method(:resolve!) - args = {} - args[:target] = targets.first if resolver.parameters.include?([:keyreq, :target]) - args[:targets] = targets if resolver.parameters.include?([:keyreq, :targets]) - - mode.resolve!(**args) + resolve_with_args(mode, target: targets.first, targets: targets) end end diff --git a/lib/magic/resolves_with_args.rb b/lib/magic/resolves_with_args.rb new file mode 100644 index 00000000..5fb4a5d6 --- /dev/null +++ b/lib/magic/resolves_with_args.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module Magic + module ResolvesWithArgs + private + + # Call receiver.resolve! passing only the keyword arguments its signature accepts. + # Callers supply the full pool of available values; this method filters to what + # the method actually declares (both required and optional keywords). + def resolve_with_args(receiver, **available) + resolver = receiver.method(:resolve!) + args = available.select do |key, _| + resolver.parameters.include?([:keyreq, key]) || + resolver.parameters.include?([:key, key]) + end + resolver.call(**args) + end + end +end