Game refactor for testability

[lphillips]

This is very much a work-in-progress of a refactor of the game logic to improve testability. The aim of this refactor is to:

• Move state management into the game module
• Reduce code duplication
• Isolate hard to test code to allow testing of the code around it

I've focused on the code around voting so far, since that is one of the main inputs that drives state changes currently. This means updating the Game.vote(player_id, vote) method and on_reaction_add(reaction, user) function, with some changes radiating out from there.

In order to centralize state transitions and have a common flow that is executed when state changes, I've introduced Game.__transition_to_state(state) to isolate assignments to Game.state and also add methods that can tell an observer when the game has entered a new state. The idea here is that the app module sends inputs to the Game via methods like vote, execute_player, nominate, etc, and that can trigger state changes. When the state has changed, the app is notified via a method like on_legislative_presidential, so it can created the appropriate embed and send it to Discord.

I've run into one hiccup here though. Messaging with Discord is asynchronous, so I can't call from the synchronous state management code in Game to the app module to update the UI. I'm very new to async programming in Python, but I'm currently thinking of setting up some async dispatch to the embed creation in response to the game callback. Something like this:

# In app.py
def on_legislative_presidential(game):
   # dispatch_to_async() is a stand-in for whatever asyncio method I can call to get it to run
   # an async method and not wait for the result. Also need to send the game through to
   # async method, however that works
    asyncio.dispatch_to_async(send_embed_for_legislative_presidential)

async def send_embed_for_legislative_presidential(game):
    # use discord API to create and send an embed
    game.finish_legislative_presidential() # this call triggers the game to move to the next state

[bsmccain]

I think asyncio.create_task() is what you want to make the async call. That's what I did in the emit() method on DiscordChannelHandler. Still reviewing, wanted to make this comment before I forgot.

[lphillips]

asyncio.create_task() seems to do the trick. I set up the code for creating a game and reacting to the game_start state change and everything happened as expected.

[lphillips]

I think this is in a state where it is ready for further review and possible merging. Here's a summary of the current state:

• game.py has been refactored to achieve the goals of reducing code duplication and making it more easily testable.
• test_game.py has an extensive suite of tests that covers a lot of the functionality in the game
• A GameStateObserver has been added to message back to the app about state transitions in the game, so the app doesn't have to make assumptions about when the state will change. There's a type-insensitive "context" parameter that gets passed through the system, so the app can pass the Discord context into the game and when the state changes, the game passes that context back via the GameStateObserver so we keep our guild context intact for sending messages.
• Image handling was factored out to a new class in order to make testing easier and to remove the duplicate code for handling images. There's no testing for this new class currently.

And a caveat:

• app.py is only partially integrated with these changes currently, so the bot won't really work with the current state of this PR. This is big enough that I'm kind of inclined to get this reviewed and merged and do a follow-up refactoring PR for the app. Open to suggestions on this though.

[lphillips]

I like the direction this is going, and approve the current implementation. I think if I were doing this from scratch, I'd probably create state objects that had methods for handling voting, transitions, etc. I'm not certain it's worth the effort in this case, but the large if/else trees seem unwieldy. But they're also implemented and functioning currently, so just rearranging is probably fine.

Yeah, I considered pulling in a state machine library and building an entirely new game engine off of that, but ultimately decided I could achieve good results by moving some of this code around and reducing duplication. A lot of the deeply nested if-statements are gone now, so it is easier to read and reason about.

[lphillips]

I addressed the smaller items from your comments. Here's what I plan to do in my next PR:

• Update GameStateObserver per your comments
• Refactor app.py to no longer reference Game.state. We'll rely on exception handling instead.
• Update VoteResult and the vote method to use exceptions and return a value that indicates if the vote was counted. Might just switch it to a bool if that makes sense.