Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor: set the message that game data was successfully loaded in a more portable place #3027

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cgolubi1
Copy link
Contributor

In particular, this change is to set the "Loaded data for game $gameId." message only in the public method load_api_game_data() which is directly invoked by the API, not in the internal method load_game().

Taking a step back: the context in which we're having so much trouble with $this->message over in #2131 is that there are two different high-level patterns of the message we want to set for the user:

  • Hi, API caller, i did something for you; i'm using the API message to say in more words "this succeeded" or "this failed", and maybe to summarize some easily-attainable details, but there's nothing more to say about it. (This is the case for both loading game data and creating a game, and lots of other things.)
  • Hi, API caller, you asked me to take a step on processing a game, and that step may have had an arbitrary number of follow-on effects, and i want to tell you about all of the ones that are relevant to you. The only way to find out what they are is to capture that information as we proceed through the state transitions. (This is the case for submitTurn.)

This confusion comes to a head with tournaments because tournaments introduce logical actions which are taken as a result of submitTurn, so as far as the site is concerned, they're part of the submitTurn workflow. But as far as the player who called submitTurn is concerned, they are not. From that player's POV, if their turn causes the end of a game which causes the next round of the tournament which causes the creation of many more games, which causes armies to be deployed and wars to be won and lost... none of that matters, they just want to know what happened during the game as a result of their attack.

So the long-term fix is to do a bigger refactor of how we set $this->message which does a better job of allowing internal methods to propagate pieces of user-friendly message text upwards, and public functions to decide what message to send.

And in the immediate term, to unblock tournaments, we can take the most limited version of that basic approach for specifically the pieces of message text that are causing problems with tournaments right now. That means:

  • Public interface methods shouldn't invoke other public interface methods, because the public interface method is designed to be called directly from the API, period.
  • $this->message should be set in the public interface method.

For the load_game() case, that second one is all that's needed, and that's what this PR does, as a proof-of-concept that it's easy and harmless to make that change, and i believe it will actually address the relevant problem in the PR.

For the create_game() case, this is showing up a slightly more complicated issue, which is that tournament game creation is calling the public create_game() method. So Shadowshade should do a refactor that moves all the logic of create_game() into a protected method, so you can have an actual public method that just makes the API call and returns it with a friendly message. That should be simple, and should get us to "public interface methods shouldn't invoke other public interface methods."

…d load_api_game_data(), not in the internal method that depends on
@cgolubi1
Copy link
Contributor Author

@cgolubi1 cgolubi1 mentioned this pull request Feb 16, 2025
35 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants