Skip to content
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

Open
wants to merge 83 commits into
base: master
Choose a base branch
from

Conversation

Middledot
Copy link
Member

@Middledot Middledot commented Aug 29, 2022

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 and ext.commands.Command are now both subclassed from Invokable. Same thing with ApplicationContext and ext.commands.Context with BaseContext.

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:

  • Remove all uses of _invoke:
    • App Contexts now have the args and kwargs attributes for argument passing. These arguments are prepared in .prepare and then passed by .invoke, just like in ext.commands.
    • Argument parsing has been moved to ._parse_arguments which is called by .prepare, just like ext.commands
  • App commands now have access to:
    • The parents, root_parent, cog_name properties
    • The .error decorator.
    • The cooldown_after_parsing attribute & parameter.
  • ApplicationContext now has the reinvoke method.
  • Ported many other 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 for SlashCommandGroup #2139 I realized this could be useful with slash commands

Other Additions:

  • BaseContext provides .source now as a way to get either message or interaction on their respective contexts.
    • Contexts now share a bunch of properties

Improvements from Parity:

  • Cooldown functionality was moved to the discord/commands folder (ext.commands.cooldown now acts as an alias).
  • Moved some command related errors to discord/errors.
    • These are also aliased in ext/commands/errors
    • Make a more organized error hierarchy.
  • Deprecated application command errors
  • Helper methods (wrap_callback, unwrap_callback, and hooked_wrapped_callback) are now not duplicated.

Other Improvements:

  • qualified_name and full_parent_name now use recursion by attributes instead of with a while loop.
  • Document ContextMenuCommand
  • Remove some of the copy functions in favor of built-in copy function (needs testing)
  • Renamed dispatch_error to _dispatch_error (and subsequently _dispatch_error is now __dispatch_error).
  • Renamed prepare for command object & help impl. to _prepare.
  • Renamed call_before_hooks & call_after_hooks in the command objects to _call_before_hooks & _call_after_hooks.

Information

  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed).
  • If code changes were made then they have been tested.
  • I have updated the documentation to reflect the changes.

@Middledot Middledot added bug Something isn't working help wanted Extra attention is needed status: in progress Work in Progess status: awaiting review Awaiting review from a maintainer feature Implements a feature labels Aug 29, 2022
Copy link
Contributor

@OmLanke OmLanke left a 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?

Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Member Author

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

Copy link
Contributor

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

@Middledot Middledot marked this pull request as draft November 6, 2023 03:10
@Middledot
Copy link
Member Author

Pushing this to v2.6

@Middledot Middledot marked this pull request as ready for review January 16, 2024 04:47
@Middledot Middledot requested review from Lulalaby, EmmmaTech and Dorukyum and removed request for BobDotCom January 16, 2024 04:50
@Lulalaby Lulalaby added this to the v2.6 milestone Feb 28, 2024
@Dorukyum Dorukyum removed the on hold label Mar 6, 2024
@Lulalaby Lulalaby modified the milestones: v2.6, v2.7 Jul 9, 2024
@Dorukyum
Copy link
Member

BaseCommand could be more fitting than Invokable.

@Lulalaby
Copy link
Member

this needs a redo, too many conflicts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feature Implements a feature help wanted Extra attention is needed on hold PA: All Contributors pending status: in progress Work in Progess
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Re-calling import in slash commands
9 participants