Enforce unique, nonempty labels for game objects#977
Conversation
d-kad
added
c++
| /// The label is set directly, without the uniqueness and nonemptiness enforcement | ||
| /// applied by SetLabel, so the file parser can assign the raw label read from a file; | ||
| /// NormalizeGameLabels makes outcome labels unique and nonempty afterwards. | ||
| virtual GameOutcome NewOutcome(const std::string &p_label = "") { throw UndefinedException(); } |
There was a problem hiding this comment.
This is smelly - we want calling code to be setting a label. This is going to be a requirement for the planned algebra of games so we want to enforce it now.
I'm also surprised that it seems to be the case that we can specify a default value here but then not in the overridden version - that seems dodgy.
It might be that we should be subdividing this PR into several independent ones - one deals with player labels, one deals with outcome labels, and so on, because each of those has plenty of wrinkles to be ironed out.
| { | ||
| m_outcomes.push_back(std::make_shared<GameOutcomeRep>(this, m_outcomes.size() + 1)); | ||
| return m_outcomes.back(); | ||
| auto outcome = std::make_shared<GameOutcomeRep>(this, m_outcomes.size() + 1); |
There was a problem hiding this comment.
See the other comment - the calling code should be providing a unique outcome label which obviates the need for doing this. We don't want anything like this to be implicit inside the game representation.
Also, the current friend structure among these classes is a code smell we're wanting to get rid of, so trading on it isn't ideal! :)
| : m_root(std::make_shared<GameNodeRep>(this, nullptr)), | ||
| m_chance(std::make_shared<GamePlayerRep>(this, 0)) | ||
| { | ||
| m_chance->m_label = "Chance"; |
There was a problem hiding this comment.
The GamePlayerRep constructor should be insisting on having a label passed, which would be set to "Chance" in the initialisation here.
I agree with the idea that we will always call the chance player "Chance" and for the moment not allowing it to be relabelled, so in effect "Chance" is a "reserved label" in extensive games.
| p = Player.wrap(self.game.deref().NewPlayer()) | ||
| if str(label) != "": | ||
| p.label = str(label) | ||
| if label is None: |
There was a problem hiding this comment.
I think I didn't outline my thinking well enough in the recent calls, because this is kind of the opposite of what I had said!
What we want is that in Python, labels need to be provided by calling code. It is up to the calling code to ensure labels are appropriately unique. Calling code might decide to write a utility function like _generate_unique_label, but that's on the calling code; it is not something we'd provide (because there's lots of different ways someone might want to generate unique labels depending on their application context).
Where we need unique label generation is in the GUI. So there would need to be a C++ helper function inside the GUI code (in appropriate places) that would generate labels. In a GUI context we are more legitimated in being prescriptive about our auto-generated labels. So we can for example default player labels to e.g. "Player 1", "Player 2", and so on, and similarly for outcomes, etc. The idea of the GUI is it trades off customisation for convenience - if users want to change from the auto-generated labels in the GUI they can do so manually.
| - In `pygambit`, indexing game object collections by integer position has been removed. (#942) | ||
| - Validity of game object labels is enforced (printable ASCII and spaces only, no leading/trailing or | ||
| double spaces); invalid labels raise `ValueError` in `pygambit`. (#944) | ||
| - Labels for game objects are now enforced in `pygambit`, completing the transition announced by the |
There was a problem hiding this comment.
Ideally ChangeLog entries should be at most two or three lines. Referring to the issue number provides more details if need be.
|
@tturocy Understood — labels are caller-supplied and enforced at the boundary; the library never generates them to satisfy the invariant. I'll rework to:
And I'll split this into per-scope PRs (players, outcomes, strategies, …). Two things to confirm:
|
|
For now let's just document how The GUI doesn't allow the chance label to be changed, so it should be safe to have |
|
Closing this as we've broken it up into individual PRs for each type of object. |
2 participants
Issues closed by this PR
Description of the changes in this PR
Enforces the label rules for game objects at the C++ level (surfaced as
ValueErrorinpygambit), replacing theFutureWarningintroduced in 16.5.0:add_player,add_outcome, andadd_strategyauto-generate a unique label ("Player n", "Outcome n", "Strategy n") when none is given, rather than requiring one.