Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions docs/prs/pr-011-resolve-with-args.md
Original file line number Diff line number Diff line change
@@ -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
2 changes: 2 additions & 0 deletions lib/magic/action.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
module Magic
class Action
include ResolvesWithArgs

attr_reader :game, :player

def initialize(game:, player:)
Expand Down
6 changes: 1 addition & 5 deletions lib/magic/actions/activate_ability.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 3 additions & 6 deletions lib/magic/actions/activate_loyalty_ability.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 7 additions & 11 deletions lib/magic/actions/cast.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand Down
9 changes: 3 additions & 6 deletions lib/magic/actions/cast/mode.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ module Magic
module Actions
class Cast < Action
class Mode
include Magic::ResolvesWithArgs

attr_reader :mode, :targets

def initialize(mode)
Expand Down Expand Up @@ -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
Expand Down
19 changes: 19 additions & 0 deletions lib/magic/resolves_with_args.rb
Original file line number Diff line number Diff line change
@@ -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
Loading