-
-
Notifications
You must be signed in to change notification settings - Fork 460
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: implement Invokable & BaseContext #1606
base: master
Are you sure you want to change the base?
Conversation
ik fake merge commit but like I can't merge anything really
* added can_run method * move cooldowns/max conc. * copy methods * compatibility fixes etc (including circular imports) * it works now btw
like bridging all the attributes and subclassed properties etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since, discord.ext.commands.Converter
is used by discord.commands.Option
too, shouldn't that be moved to the discord.commands
folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If discord.ext.commands.cooldowns
was moved to discord.commands.cooldowns
, shouldn't the decorators like @discord.ext.commands.cooldown()
from this file move to discord.commands.core
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even the decorators for before_invoke
and after_invoke
from this file should be moved to discord.commands.core
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense to move this stuff, but I looked around, and moving *_invoke and cooldown decors would imply the moving of a LOT of other things from ext to the main folder. App commands also use max_conc, the various checks, and (technically?) permission validators. I'd prefer this be done in another pr, because this pr is old and big enough as it is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that could be a separate PR. This one is actually the oldest open PR xD. Lots of merge conflicts too ig
Signed-off-by: Middledot <[email protected]>
Pushing this to v2.6 |
Signed-off-by: Middledot <[email protected]>
BaseCommand could be more fitting than Invokable. |
this needs a redo, too many conflicts |
Description
Even though app commands and text commands share many features, some are only implemented on one of the two AND a huge chunk of these features get re-implemented each time they get used. Invokables make it easier to share common features between the 2. BaseContext does the same but for contexts.
ApplicationCommand
andext.commands.Command
are now both subclassed fromInvokable
. Same thing withApplicationContext
andext.commands.Context
withBaseContext
.This tries to be as least breaking as possible btw.
But should probably be tested before merging.
Now that I see it, this is kind of related to #387
Summary
(Possible) breaking/Important changes are highlighted
Additions because of Parity:
_invoke
:args
andkwargs
attributes for argument passing. These arguments are prepared in.prepare
and then passed by.invoke
, just like in ext.commands.._parse_arguments
which is called by.prepare
, just like ext.commandsparents
,root_parent
,cog_name
properties.error
decorator.cooldown_after_parsing
attribute & parameter.ApplicationContext
now has thereinvoke
method.ext.commands.Context
misc attributes related to invocation.App commands do not run their parent's checks anymore for parity.With fix: run only local hooks forSlashCommandGroup
#2139 I realized this could be useful with slash commandsOther Additions:
BaseContext
provides.source
now as a way to get either message or interaction on their respective contexts.Improvements from Parity:
discord/commands
folder (ext.commands.cooldown now acts as an alias).wrap_callback
,unwrap_callback
, andhooked_wrapped_callback
) are now not duplicated.Other Improvements:
qualified_name
andfull_parent_name
now use recursion by attributes instead of with a while loop.ContextMenuCommand
dispatch_error
to_dispatch_error
(and subsequently_dispatch_error
is now__dispatch_error
).Renamedprepare
for command object & help impl. to_prepare
.Renamedcall_before_hooks
&call_after_hooks
in the command objects to_call_before_hooks
&_call_after_hooks
.Information