diff --git a/src/games/game.h b/src/games/game.h index 286ddf12b..4cbbcecf2 100644 --- a/src/games/game.h +++ b/src/games/game.h @@ -369,11 +369,7 @@ class GameStrategyRep : public std::enable_shared_from_this { /// Returns the text label associated with the strategy const std::string &GetLabel() const { return m_label; } /// Sets the text label associated with the strategy - void SetLabel(const std::string &p_label) - { - CheckLabel(p_label); - m_label = p_label; - } + void SetLabel(const std::string &p_label); /// Returns the game on which the strategy is defined Game GetGame() const; @@ -492,6 +488,8 @@ class GamePlayerRep : public std::enable_shared_from_this { GameStrategy GetStrategy(int st) const; /// Returns the collection of strategies available to the player Strategies GetStrategies() const; + /// Validate that p_label is a nonempty, valid, unique label for a strategy of this player. + void CheckStrategyLabel(const std::string &p_label) const; //@} /// @name Sequences @@ -1311,6 +1309,27 @@ inline void GameOutcomeRep::SetPayoff(const GamePlayer &p_player, const Number & inline GamePlayer GameStrategyRep::GetPlayer() const { return m_player->shared_from_this(); } inline Game GameStrategyRep::GetGame() const { return m_player->GetGame(); } +inline void GameStrategyRep::SetLabel(const std::string &p_label) +{ + if (p_label == m_label) { + return; + } + GetPlayer()->CheckStrategyLabel(p_label); + m_label = p_label; +} + +inline void GamePlayerRep::CheckStrategyLabel(const std::string &p_label) const +{ + if (p_label.empty()) { + throw ValueException("Strategy label must not be empty"); + } + CheckLabel(p_label); + for (const auto &strategy : m_strategies) { + if (strategy->GetLabel() == p_label) { + throw ValueException("Strategy label must be unique for the player"); + } + } +} inline Game GameSequenceRep::GetGame() const { return m_player->GetGame(); } inline GamePlayer GameSequenceRep::GetPlayer() const { return m_player->shared_from_this(); } diff --git a/src/games/gametable.cc b/src/games/gametable.cc index 078969d1a..cf373e319 100644 --- a/src/games/gametable.cc +++ b/src/games/gametable.cc @@ -531,13 +531,15 @@ GameStrategy GameTableRep::NewStrategy(const GamePlayer &p_player, const std::st if (p_player->GetGame().get() != this) { throw MismatchException(); } + p_player->CheckStrategyLabel(p_label); + auto strategy = std::make_shared(p_player.get(), + p_player->m_strategies.size() + 1, p_label); IncrementVersion(); std::vector old_radices; for (const auto &player : m_players) { old_radices.push_back(player->m_strategies.size()); } - p_player->m_strategies.push_back(std::make_shared( - p_player.get(), p_player->m_strategies.size() + 1, p_label)); + p_player->m_strategies.push_back(strategy); RebuildTable(old_radices); return p_player->m_strategies.back(); } diff --git a/src/gui/gamedoc.cc b/src/gui/gamedoc.cc index f971996e2..49154d9f1 100644 --- a/src/gui/gamedoc.cc +++ b/src/gui/gamedoc.cc @@ -497,7 +497,15 @@ void GameDocument::DoSetPlayerLabel(GamePlayer p_player, const wxString &p_label void GameDocument::DoNewStrategy(GamePlayer p_player) { - m_game->NewStrategy(p_player, std::to_string(p_player->GetStrategies().size() + 1)); + std::set strategyLabels; + for (const auto &strategy : p_player->GetStrategies()) { + strategyLabels.insert(strategy->GetLabel()); + } + int number = p_player->GetStrategies().size() + 1; + while (contains(strategyLabels, std::to_string(number))) { + number++; + } + m_game->NewStrategy(p_player, std::to_string(number)); NotifyChanged(GameModificationType::GameForm); } diff --git a/src/pygambit/game.pxi b/src/pygambit/game.pxi index 403d095d8..3f05b6458 100644 --- a/src/pygambit/game.pxi +++ b/src/pygambit/game.pxi @@ -2173,15 +2173,20 @@ class Game: resolved_outcome = cython.cast(Outcome, self._resolve_outcome(outcome, "set_outcome")) self.game.deref().SetOutcome(resolved_node.node, resolved_outcome.outcome) - def add_strategy(self, player: Player | str, label: str = None) -> Strategy: + def add_strategy(self, player: Player | str, label: str) -> Strategy: """Add a new strategy to the set of strategies for `player`. + .. versionchanged:: 16.7.0 + A label is now required and must be nonempty and unique among the + player's strategies. + Parameters ---------- player : Player or str The player to create the new strategy for - label : str, optional - The label to assign to the new strategy + label : str + The label for the new strategy. Must be nonempty and not already in use + by another of the player's strategies. Returns ------- @@ -2194,6 +2199,8 @@ class Game: If `player` is a `Player` from a different game. UndefinedOperationError If called on a game which has an extensive representation. + ValueError + If `label` is empty or is already the label of another of the player's strategies. """ if self.is_tree: raise UndefinedOperationError( @@ -2201,9 +2208,8 @@ class Game: ) resolved_player = cython.cast(Player, self._resolve_player(player, "add_strategy")) - label_bytes = (str(label) if label is not None else "").encode("ascii") return Strategy.wrap( - self.game.deref().NewStrategy(resolved_player.player, label_bytes) + self.game.deref().NewStrategy(resolved_player.player, label.encode("ascii")) ) def delete_strategy(self, strategy: Strategy | str) -> None: diff --git a/src/pygambit/strategy.pxi b/src/pygambit/strategy.pxi index f46602b8b..80a605460 100644 --- a/src/pygambit/strategy.pxi +++ b/src/pygambit/strategy.pxi @@ -55,18 +55,15 @@ class Strategy: """Get or set the text label associated with the strategy. .. versionchanged:: 16.7.0 - An invalid label now raises ``ValueError``: a label may contain only printable ASCII - characters and spaces, not begin/end with a space, nor have two consecutive spaces. + A strategy label must be nonempty and unique among the player's strategies; + an empty or duplicate label now raises ``ValueError``. A label may contain only + printable ASCII characters and spaces, not begin/end with a space, nor have two + consecutive spaces. """ return self.strategy.deref().GetLabel().decode("ascii") @label.setter def label(self, value: str) -> None: - if value == self.label: - return - if value == "" or value in (strategy.label for strategy in self.player.strategies): - warnings.warn("In a future version, strategies for a player must have unique labels", - FutureWarning) self.strategy.deref().SetLabel(value.encode("ascii")) @property diff --git a/tests/test_players.py b/tests/test_players.py index bcc10b026..3d2da5b52 100644 --- a/tests/test_players.py +++ b/tests/test_players.py @@ -145,6 +145,27 @@ def test_add_strategy_label_invalid_raises_valueerror(label): game.add_strategy(next(iter(game.players)), label) +def test_add_strategy_requires_label(): + game = gbt.Game.new_table([2, 2]) + with pytest.raises(TypeError): + game.add_strategy(next(iter(game.players))) + + +def test_strategy_label_empty_raises_valueerror(): + game = gbt.Game.new_table([2, 2]) + strategy = next(iter(next(iter(game.players)).strategies)) + with pytest.raises(ValueError): + strategy.label = "" + + +def test_strategy_label_duplicate_within_player_raises_valueerror(): + game = gbt.Game.new_table([2, 2]) + pl1 = next(iter(game.players)) + s1, s2 = pl1.strategies + with pytest.raises(ValueError): + s2.label = s1.label + + def test_player_strategy_bad_label(): game = gbt.Game.new_table([2, 2]) pl1 = next(iter(game.players)) @@ -232,7 +253,7 @@ def test_player_get_min_payoff_null_outcome(): pl1, pl2 = game.players assert pl1.min_payoff == 1 assert pl2.min_payoff == 2 - game.add_strategy(pl1) + game.add_strategy(pl1, "new strategy") # Currently the outcomes associated with the new entries in the table # are null outcomes. So now minimum payoff should be zero from those. for player in game.players: @@ -258,8 +279,27 @@ def test_player_get_max_payoff_null_outcome(): pl1, pl2 = game.players assert pl1.max_payoff == -1 assert pl2.max_payoff == -2 - game.add_strategy(pl1) + game.add_strategy(pl1, "new strategy") # Currently the outcomes associated with the new entries in the table # are null outcomes. So now minimum payoff should be zero from those. for player in game.players: assert player.max_payoff == 0 + + +def test_add_strategy_duplicate_label_raises_and_leaves_game_unchanged(): + game = gbt.Game.new_table([2, 2]) + pl = next(iter(game.players)) + existing = next(iter(pl.strategies)).label + count_before = len(pl.strategies) + with pytest.raises(ValueError): + game.add_strategy(pl, existing) + assert len(pl.strategies) == count_before + + +def test_add_strategy_empty_label_raises_and_leaves_game_unchanged(): + game = gbt.Game.new_table([2, 2]) + pl = next(iter(game.players)) + count_before = len(pl.strategies) + with pytest.raises(ValueError): + game.add_strategy(pl, "") + assert len(pl.strategies) == count_before