Skip to content

Proposal: Using Enums for command switches#399

Closed
LeBulldoge wants to merge 2 commits intoArx-Game:stable_orphanfrom
LeBulldoge:alternative_switch
Closed

Proposal: Using Enums for command switches#399
LeBulldoge wants to merge 2 commits intoArx-Game:stable_orphanfrom
LeBulldoge:alternative_switch

Conversation

@LeBulldoge
Copy link
Contributor

@LeBulldoge LeBulldoge commented Jul 6, 2021

Brief overview of PR changes/additions

An example of an alternative for how Arx handles command switches.

Motivation for adding to Arx

After spending some time exploring the code for various commands, it seems to me that the current way Evennia handles command switches tends to completely and utterly destroy code consistency and readability. Jumping into code for a command you've yet to see, is like diving into a minefield. Standardizing the way we handle switches should drastically improve that and Enums seem like the perfect candidate for the job, especially whenever we decide to move to Python 3.10, which adds match statements, making handling this implementation even more of a breeze.

As enums require a default value, that means that we could enforce providing documentation for every switch:

class Switches(Enum):
    followup = "<#>=<message> | Add a followup message to a request."
...

This would make helpfiles much more consistent for both the player and the developer.
I provided an example of that in the PR as well.

Thankfully, Evennia already provides us with switch_options in MuxCommand, however, that field expects a list of strings, but changing that is extremely easy, especially since we use a fork of Evennia anyway.

As it stands, this implementation wouldn't break any commands that don't utilize self.switch_options already, letting us port them over at our own pace. In the future, self.switches could also be changed to provide us with a list of switches already casted to the Enum provided in self.switch_options.

@TehomCD
Copy link
Member

TehomCD commented Jul 13, 2021

This doesn't appear to be working:

'Traceback (most recent call last):\n  File "/home/runner/work/arxcode/arxcode/server/utils/test_utils.py", line 231, in call\n    cmdobj.parse()\n  File "/home/runner/work/arxcode/evennia/evennia/commands/default/muxcommand.py", line 118, in parse\n    self.switch_options = [opt.lower() for opt in self.switch_options]\n  File "/home/runner/work/arxcode/evennia/evennia/commands/default/muxcommand.py", line 118, in <listcomp>\n    self.switch_options = [opt.lower() for opt in self.switch_options]\nAttributeError: \'Switches\' object has no attribute \'lower\''

I've never been very fond of Enums. They always seem to result in errors much like the above when you intuitively try to use the thing itself rather than specifying the name or value property. I agree that switches should be more formalized, and Enums do make sense for that, but in this case it looks like you might need to add an __iter__ that returns a generator of the .name of each of its members or some such. It might be cleanest to make a SwitchesEnum base class that has the required functionality it'd need to be passed to switch_options and handled properly by muxcommand.

@LeBulldoge
Copy link
Contributor Author

LeBulldoge commented Jul 13, 2021

Oh, this PR is more of an example of a way we could possibly use them. It wouldn't work as is, because as I mentioned:

Thankfully, Evennia already provides us with switch_options in MuxCommand, however, that field expects a list of strings, but changing that is extremely easy, especially since we use a fork of Evennia anyway.

Thinking back, I guess I should've put this in as an issue instead.
Overriding __iter__ is a better idea though, could certainly be done if you think that this is worth to continue working on and utilize in the project.

@LeBulldoge
Copy link
Contributor Author

Looks like we could just use the __members__ property already present in the Enum class, though I'd still make a base class and wrap its use with a method.

@LeBulldoge
Copy link
Contributor Author

Or, the self.switch_options field could be ignored completely, I've only suggested using it to further standardization, It seems you don't really use it across the project to begin with. Defining an enum within the class should suffice.

@TehomCD
Copy link
Member

TehomCD commented Jul 13, 2021

Yeah, long term what I'd really want is something equivalent to a DRF viewset where the syntax and helpfile would be built from the different methods you provide, sorta like using the @action decorator on viewset methods. Lemme make an issue for that and link back to this, and you could close this unless you feel like getting an enum class working as an example implementation

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