[Commands] Refactor command and group decorators #1818
Conversation
…factor_cmd_decorators # Conflicts: # redbot/core/bot.py
…factor_cmd_decorators # Conflicts: # redbot/core/commands/commands.py
redbot/core/bot.py
Outdated
| @@ -24,7 +22,7 @@ | |||
| from .sentry import SentryManager | |||
|
|
|||
|
|
|||
| class RedBase(BotBase, RPCMixin): | |||
| class RedBase(commands.GroupMixin, commands.bot.BotBase, RPCMixin): | |||
There was a problem hiding this comment.
Why call our own reference to BotBase? I don't see a whole lot of sense in doing this
There was a problem hiding this comment.
We can change it to discord.ext.commands.bot.BotBase if you'd like, it wouldn't make a difference
There was a problem hiding this comment.
I could instead subclass discord.ext.commands.bot.BotBase and add the GroupMixin there. It wouldn't be any different to the current setup but would simplify RedBase's direct inheritance
There was a problem hiding this comment.
See here for an example of what I mean, I don't really think its necessary but all of this inheritance stuff is pretty confusing to some, perhaps this makes it a bit simpler? I'm not sure.
There was a problem hiding this comment.
Sorry, this is probably a better way to show the example
|
Due to my inexperience I'm not gonna touch this in Beta 17. Todo for Beta 18 |
There was a problem hiding this comment.
Everything in the bot appears to be fine with these changes, but I do think a tweak to the mro here should be made.
Here's the mro for commands.group:
(<class 'redbot.core.commands.commands.Group'>,
<class 'redbot.core.commands.commands.Command'>,
<class 'redbot.core.commands.commands.GroupMixin'>,
<class 'discord.ext.commands.core.Group'>,
<class 'discord.ext.commands.core.GroupMixin'>,
<class 'discord.ext.commands.core.Command'>,
<class 'object'>)I'm very hesitant to have our explicit overrides have a different ordering here than discord.py's bases without extremely good reason for doing so, and that reason being documented for future maintaining. While it doesn't seem to matter currently, it seems a recipe for potential subtle, hard to debug breakage later.
|
I've rebased and fixed the MRO in https://github.com/calebj/Red-DiscordBot/tree/v3/refactor_cmd_decorators |
|
Thanks @calebj and @mikeshardmind, I've tweaked the MRO accordingly |
Tobotimus
added
the
QA: Passed
Tobotimus
changed the title
Jackenmen
added
the
Type: Bug
Curiously, these decorators were previously placed in the
Commandclass. Whilst it hadn't caused any issues yet, there's no reason for them to be there. This puts them in a new overriddenGroupMixinclass, so that the bot also inherits these decorators.I also added some tests as a sanity check, and also to make sure this shit doesn't break in the future.