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

Closed
wants to merge 83 commits into 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
@Middledot Middledot requested review from Lulalaby and plun1331 August 29, 2022 22:52
@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

@Lulalaby
Copy link
Member

This is outdated and needs a rework - Closing

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 status: in progress Work in Progess
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Re-calling import in slash commands
9 participants