hohav / py-slippi

Python library for parsing SSBM replay files
MIT License
56 stars 25 forks source link

flatten classes #35

Closed DustinAlandzes closed 3 years ago

DustinAlandzes commented 3 years ago

I don't think defining the classes within other classes is standard

open to using different names

hohav commented 3 years ago

Thank you for the PR, but please open a discussion issue before working on a large refactor like this.

Re: "standard", nested/inner classes aren't super common, but they're hardly obscure and I didn't use them by accident. My intent was to provide organization for the many data classes the parser emits. This could be done in other ways (like a deep namespace hierarchy), but I'm not planning to switch without seeing some concrete advantages over the current approach.

DustinAlandzes commented 3 years ago

This is what triggered this PR

https://github.com/hohav/py-slippi/blob/master/test/replays.py#L13-L14

BPhys = Buttons.Physical
BLog = Buttons.Logical

The nested classes are uncommon and hard to read/use

It causes terrible test code like this

self.assertEqual(self._game('shield_drop').start.players[0].ucf, Start.Player.UCF(dash_back=Start.Player.UCF.DashBack.OFF, shield_drop=Start.Player.UCF.ShieldDrop.UCF))
        self.assertEqual(self._game('dash_back').start.players[0].ucf, Start.Player.UCF(dash_back=Start.Player.UCF.DashBack.UCF, shield_drop=Start.Player.UCF.ShieldDrop.OFF))
hohav commented 3 years ago

Uncommon maybe, but I don't see how they are hard to read. TriggersPhysical does not seem easier to read than Triggers.Physical, and in fact I find the latter more legible due to the segmentation.

BPhys is just a short alias, which I would have used regardless of whether the actual class name was Buttons.Physical or ButtonsPhysical. It's little different from a hypothetical from slippi.buttons import Physical as BPhys.

Which also brings up the problem that slippi.event.Physical is an ambiguous name. That's a large part of why I used nested classes: the actual class names can be shorter because they're embedded in a more limited context than just a module.

A similar effect could be achieved with a more granular set of modules (e.g. slippi/buttons.py, slippi/triggers.py, ...), and I would consider a PR that made such a change— but if you do plan to create one, please submit your proposed module organization in an issue first.

I also don't see what's terrible about that test code. If it's just the verbosity, you can just do e.g. UCF = Start.Player.UCF (again, not much different from from slippi.start.player import UCF or whatever).