refactor: store invokable application commands properly #1107

EQUENOS · 2023-09-18T12:31:28Z

Summary

Currently, disnake.ext.commands framework doesn't allow registering 2 guild commands with the same type and name in different guilds, even though discord API allows to do that (see issue #260). This PR solves this problem.

Consequences

all_xxx_commands attributes of InteractionBotBase are now deprecated in favor of all_app_commands
bot.add_xxx_command methods are now deprecated in favor of add_app_command
bot.remove_xxx_command methods are now deprecated in favor of remove_app_command
bot.get_xxx_command now works in O(1) only if guild_id is specified, otherwise it's O(n) where n = len(all_app_commands)
bot.get_xxx_command can be ambiguous if guild_id is not specified
argument guild_id of bot._ordered_unsynced_commands is now deprecated

Note

The original idea of allowing to insert IDs of app commands to the corresponding invokable objects had to be abandoned. This is due to complications related to copying.

Checklist

If code changes were made, then they have been tested
- I have updated the documentation to reflect the changes
- I have formatted the code properly by running pdm lint
- I have type-checked the code by running pdm pyright
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)
This PR is not a code change (e.g. documentation, README, ...)

disnake/interactions/application_command.py

disnake/ext/commands/interaction_bot_base.py

shiftinv

I have a bunch of mostly minor comments, but overall this looks really cool 👀

changelog/260.deprecate.0.rst

disnake/ext/commands/errors.py

disnake/ext/commands/interaction_bot_base.py

docs/ext/commands/api/exceptions.rst

disnake/interactions/application_command.py

shiftinv · 2023-09-22T14:20:50Z

Haven't tested much yet - not sure what exactly happened, but something somewhere ended up trying to register subcommands as top-level commands: https://gist.github.com/shiftinv/eddfcae9ca1f34790ced4a5288e3691f

Also adds `alias_conflict` attribute for backwards compatibility

EQUENOS · 2023-09-23T22:39:49Z

Haven't tested much yet - not sure what exactly happened, but something somewhere ended up trying to register subcommands as top-level commands: https://gist.github.com/shiftinv/eddfcae9ca1f34790ced4a5288e3691f

Fixed this in 1f10a8d

disnake/ext/commands/interaction_bot_base.py

disnake/ext/commands/__init__.py

shiftinv · 2023-12-11T20:12:05Z

disnake/ext/commands/interaction_bot_base.py

+            )
+
+        test_guilds = (None,) if self._test_guilds is None else self._test_guilds
+        guild_ids = app_command.guild_ids or test_guilds


We should add a note to the .name and .guild_ids attributes that any changes after adding the command to the bot (e.g. through @bot.slash_command, bot.add_cog, etc.) are not supported and may result in undefined behavior.
I know of one instance where cmd.guild_ids is modified in a cog constructor, but from what I can tell that'll still work fine even after this change, since it runs before add_cog gets called.

shiftinv · 2023-12-14T13:04:51Z

disnake/ext/commands/cog.py

            for app_command in self.__cog_app_commands__:
-                if isinstance(app_command, InvokableSlashCommand):
-                    bot.remove_slash_command(app_command.name)
-                elif isinstance(app_command, InvokableUserCommand):
-                    bot.remove_user_command(app_command.name)
-                elif isinstance(app_command, InvokableMessageCommand):
-                    bot.remove_message_command(app_command.name)
+                bot._remove_app_commands(
+                    app_command.body.type, app_command.name, guild_ids=app_command.guild_ids
+                )


This doesn't pose an issue right now, but may need to check for SubCommand(Group) here too.

shiftinv · 2023-12-14T13:06:32Z

disnake/interactions/application_command.py

+    guild_id: Optional[:class:`int`]
+        ID of the guild the command is registered to.


Suggested change

guild_id: Optional[:class:`int`]

ID of the guild the command is registered to.

guild_id: Optional[:class:`int`]

ID of the guild the command is registered to.

.. versionadded:: 2.10

shiftinv · 2023-12-14T13:26:19Z

disnake/ext/commands/interaction_bot_base.py

+            # localization may be called multiple times for the same command but it's harmless
+            app_command.body.localize(self.i18n)


As you mentioned in the comment, calling this multiple times is perfectly fine, but could this just be moved out of the loop instead?

shiftinv · 2023-12-14T13:40:23Z

disnake/ext/commands/interaction_bot_base.py

+        guild_id: Optional[:class:`int`]
+            The ID of the guild from which this command should be removed,
+            or ``None`` if it's global.


Suggested change

guild_id: Optional[:class:`int`]

The ID of the guild from which this command should be removed,

or ``None`` if it's global.

guild_id: Optional[:class:`int`]

The ID of the guild from which this command should be removed,

or ``None`` if it's global. If ``test_guilds`` is specified in the bot constructor,

passing ``None`` here will remove the command from those guilds instead.

shiftinv · 2023-12-14T13:49:49Z

disnake/ext/commands/interaction_bot_base.py


    def get_slash_command(
-        self, name: str
+        self, name: str, guild_id: Optional[int] = MISSING


small nit, same for the two methods below

Suggested change

self, name: str, guild_id: Optional[int] = MISSING

self, name: str, *, guild_id: Optional[int] = MISSING

EQUENOS added 3 commits September 17, 2023 00:23

A draft of the app commands storage refactor

3c37179

Add missing deprecation warnings and properties

b650009

Fix a bad check

ec3896d

EQUENOS added p: medium Medium priority t: refactor/typing/lint Refactors, typing changes and/or linting changes s: in progress Issue/PR is being worked on labels Sep 18, 2023

Fix linting issues

ea1ad65

EQUENOS linked an issue Sep 18, 2023 that may be closed by this pull request

Separate application commands with same name+type but in different guilds don't work #260

Open

3 tasks

Update docs and add changelog

424ed03

shiftinv mentioned this pull request Sep 18, 2023

refactor app command sync and other fixes #666

Closed

8 tasks

EQUENOS changed the title ~~refactor: Store invokable application commands properly~~ refactor: store invokable application commands properly Sep 18, 2023

EQUENOS added 2 commits September 18, 2023 16:31

Avoid unnecessary generators

a2b0677

Fix docs references

dbd7920

EQUENOS added s: needs review Issue/PR is awaiting reviews t: deprecation Deprecation of existing features t: bugfix and removed s: in progress Issue/PR is being worked on labels Sep 18, 2023

Skelmis reviewed Sep 18, 2023

View reviewed changes

EQUENOS added 2 commits September 19, 2023 23:05

Add errors for ambiguous app command searches

e86e5cb

First word of the docstring should not be "This"

f80dbf6

shiftinv reviewed Sep 22, 2023

View reviewed changes

EQUENOS added 6 commits September 22, 2023 23:08

docs: group some changelog entries

4ed930d

chore: rename AppCommandRegistrationError

192d2b9

Also adds `alias_conflict` attribute for backwards compatibility

docs: fix references in changelog entries

686bd48

docs: hide the ext.commands prefix in changelog entries

8b5916c

docs: revert the removal of ext.commands prefix

88584c7

fix: subcommands trying to be top level commands

1f10a8d

chore: remove deprecated undocumented things

f5d5d57

EQUENOS added 6 commits September 24, 2023 01:48

chore: fix a small linting issue

164df50

chore: replace warn_deprecated with deprecated

0064e9a

chore: resolve remove_app_command nits

4736dad

chore: add suggested typings

8c6bb90

docs: fix typos and improve comments

c44806b

chore: simplify ambiguity checks

ef591e6

EQUENOS requested a review from shiftinv September 24, 2023 22:14

docs: explain command location priorities

42c7e6f

shiftinv reviewed Oct 8, 2023

View reviewed changes

disnake/ext/commands/interaction_bot_base.py Outdated Show resolved Hide resolved

disnake/ext/commands/interaction_bot_base.py Outdated Show resolved Hide resolved

disnake/ext/commands/interaction_bot_base.py Show resolved Hide resolved

shiftinv mentioned this pull request Nov 3, 2023

fix: Another blind spot of command sync algorithm #1108 #1129

Open

8 tasks

EQUENOS added 7 commits November 3, 2023 18:31

Make bot.all_app_commands a mapping proxy

9a4d1aa

fix: unintended breaking change (handling guild_ids=[])

55299b2

docs: AppCmdIndex

8dc756e

fix: docs and linting

d519711

Make AppCmdIndex public

413f759

docs: fix a reference

304a829

docs: improve AppCmdIndex docstring

63ce007

shiftinv reviewed Dec 9, 2023

View reviewed changes

disnake/ext/commands/interaction_bot_base.py Outdated Show resolved Hide resolved

disnake/ext/commands/interaction_bot_base.py Outdated Show resolved Hide resolved

disnake/ext/commands/__init__.py Outdated Show resolved Hide resolved

EQUENOS added 5 commits December 11, 2023 18:14

docs: small fixes; chore: move AppCmdIndex to a different file

a580238

Merge branch 'master' into refactor/app-command-storage

764324a

fix: prevent an unintended breaking change

ebdeb33

refactor: remove_app_command now takes a single guild_id

93880cf

refactor: use @deprecated for add_xxx_command

84eff42

shiftinv reviewed Dec 14, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: store invokable application commands properly #1107

refactor: store invokable application commands properly #1107

EQUENOS commented Sep 18, 2023 •

edited

Loading

shiftinv left a comment

shiftinv commented Sep 22, 2023

EQUENOS commented Sep 23, 2023

shiftinv Dec 11, 2023

shiftinv Dec 14, 2023

shiftinv Dec 14, 2023

shiftinv Dec 14, 2023

shiftinv Dec 14, 2023

shiftinv Dec 14, 2023

		guild_id: Optional[:class:`int`]
		ID of the guild the command is registered to.

		# localization may be called multiple times for the same command but it's harmless
		app_command.body.localize(self.i18n)

	self, name: str, guild_id: Optional[int] = MISSING
	self, name: str, *, guild_id: Optional[int] = MISSING

refactor: store invokable application commands properly #1107

Are you sure you want to change the base?

refactor: store invokable application commands properly #1107

Conversation

EQUENOS commented Sep 18, 2023 • edited Loading

Summary

Consequences

Note

Checklist

shiftinv left a comment

Choose a reason for hiding this comment

shiftinv commented Sep 22, 2023

EQUENOS commented Sep 23, 2023

shiftinv Dec 11, 2023

Choose a reason for hiding this comment

shiftinv Dec 14, 2023

Choose a reason for hiding this comment

shiftinv Dec 14, 2023

Choose a reason for hiding this comment

shiftinv Dec 14, 2023

Choose a reason for hiding this comment

shiftinv Dec 14, 2023

Choose a reason for hiding this comment

shiftinv Dec 14, 2023

Choose a reason for hiding this comment

EQUENOS commented Sep 18, 2023 •

edited

Loading