Enforce unique, nonempty labels for players - #979
Conversation
d-kad
added
c++
|
I have pushed some fixes to the GUI. The previous changes were assuming that "Player (n+1)" would necessarily be a unique label, which it might not be. This corrects that (and centralises the unique label generation to a single site). In the Python tests, we don't have a test to confirm that trying to create a player with a duplicate or empty label fails, and further that doing so leaves the game unchanged. |
| if str(label) != "": | ||
| p.label = str(label) | ||
| return p | ||
| label_str = str(label) |
There was a problem hiding this comment.
I'm not sure what calling str() on what is already guaranteed to be a str() is intended to do?
| label_str = str(label) | ||
| if not label_str: | ||
| raise ValueError("add_player(): label must not be empty") | ||
| existing = [player.label for player in self.players] |
There was a problem hiding this comment.
All these checks duplicate what is already supposed to be done in NewPlayer. And in fact they mask a bug: C++ NewPlayer allows the creation of a player with the label "Chance".
| GamePlayer GameTreeRep::NewPlayer() | ||
| GamePlayer GameTreeRep::NewPlayer(const std::string &p_label) | ||
| { | ||
| auto player = std::make_shared<GamePlayerRep>(this, m_players.size() + 1, ""); |
There was a problem hiding this comment.
This defeats the purpose of having the label as an argument of the ctor if we allow ourselves to create a player with an invalid label.
What should be done is the validity of the label is checked first, and then the object allocated after the validity is confirmed.
This means that the logic in SetLabel for duplicate checking needs to be its own function that gets called from wherever is relevant.
2 participants
Issues closed by this PR
Description of the changes in this PR
Enforces the label rules for players, following #978 (nodes and information sets). Players are a non-empty, unique-within-game scope, enforced in
GamePlayerRep::SetLabel:ValueError."Chance", set through the constructor, and its label is reserved:SetLabelrejects any attempt to relabel it.add_playernow requires a label (no default, no auto-generation).