Unifying `Battle` and `DoubleBattle`

[cameronangliss]

This is a follow-up to the work done in #399. One of the difficult parts of that MR was trying to give types to many of the abstract methods of the AbstractBattle class, which, when inherited by Battle and DoubleBattle, have different types that could likely be unified if it were structured under a more general Battle class. This would greatly enhance the experience of the users, since it may be somewhat confusing how to write the choose_move method since it can take in either a Battle type or DoubleBattle type, and those types have similarly-named attributes with different return types. I saw a card in "Projects" under "Poke-env - general" that seems relevant to this: https://github.com/hsahovic/poke-env/projects/2#card-70518735. I just wanted to get the ball rolling to figure out how to handle this - thanks!

[hsahovic]

That sounds like a good project - what changes would you like to implement, at a high-level?

[cameronangliss]

So my vision would essentially be to replace the AbstractBattle, Battle, and DoubleBattle files with a single Battle file which has a Battle class that is generalized for single or double battles. Since double battles are kind of like a listed version of singles battles, we could simply always return lists for things like the active_pokemon property, even when it's a singles battle, in which case you would always just index into the 0th element to get the active pokemon. This is basically the system that's used by Showdown's request messages, which is probably a good precedent to follow. This would allow us to do away with a lot of un-DRY code and unify the type system a lot more, but I also understand that this change could be seen as quite invasive. Let me know what you think though, and also if this answered your question.

[hsahovic]

I think this overall makes sense, but i think the API should be kept as simple as possible. For instance, returning a list of length one for active_pokemon in a single battle is probably not optimal. We could store _active_pokemon as a list, and return only the first value in single battles though. What do you think?

[cameronangliss]

I've had another idea now actually. Instead of unifying Battle and DoubleBattle, it may make more sense to simply have two methods, choose_singles_move and choose_doubles_move, in the Player class, rather than just one choose_move method. This would address my concerns entirely, while not tinkering too much with the API as it stands. Essentially, I'm proposing the following API change:

Before: def choose_move(self, battle: AbstractBattle) -> Union[BattleOrder, Awaitable[BattleOrder]]:

After: def choose_singles_move(self, battle: Battle) -> Union[BattleOrder, Awaitable[BattleOrder]]: def choose_doubles_move(self, battle: DoubleBattle) -> Union[BattleOrder, Awaitable[BattleOrder]]:

We could even leave choose_move around for legacy, and just add the other two methods for people who want something that's a little more specific case-wise.

[cameronangliss]

Update: I tried quickly prototyping such a change in #429, but I was unable to keep choose_move around for legacy in an elegant way. It would require finding some way to make it so that if the user has implemented choose_move, they don't need to implement choose_singles_move or choose_doubles_move anymore, and vice-versa. I can't find a good way to do this in Python without introducing new jagged edges to the interface, so I believe it may be best to simply replace choose_move with the pair of methods choose_singles_move and choose_doubles_move.

[cameronangliss]

Another update: I'm realizing now that it may not be necessary to change anything at all. Consider the following code:

class MyPlayer(Player):
    def choose_move(self, battle: AbstractBattle) -> BattleOrder:
        if isinstance(battle, Battle):
            return self.choose_singles_move(battle)
        elif isinstance(battle, DoubleBattle):
            return self.choose_doubles_move(battle)
        else:
            raise ValueError(f"battle should be Battle or DoubleBattle. Received {type(battle)}")

    def choose_singles_move(self, battle: Battle) -> BattleOrder:
        # implementation here
        pass

    def choose_doubles_move(self, battle: DoubleBattle) -> BattleOrder:
        # implementation here
        pass

It may be a good idea to make a file in the examples folder demonstrating this technique (if one doesn't already exist), but I think I'm happy with this approach. Let me know what you think.

[cameronangliss]

I think this issue can be safely closed. The above solution is completely sufficient to resolve my initial concerns. Thanks for your time @hsahovic!