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.

Sign up for GitHub

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

Jump to bottom

refactor app command sync and other fixes #666

Closed

onerandomusername wants to merge 19 commits into DisnakeDev:master from onerandomusername:refactor/app-commands

Member

onerandomusername commented Jul 27, 2022 •

edited

Loading

Summary

Refactors app command sync and lays the groundwork to implement GH-665 (but doesn't do it as of now)

closes Separate application commands with same name+type but in different guilds don't work #260
closes Add way to mention slash commands #631
closes support renaming app commands #783 (not yet implemented)

Checklist

I have not extensively tested or caught most cases, but a review (even in this condition) would be appreciated

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 task lint
- I have type-checked the code by running task 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, ...)

onerandomusername added t: enhancement breaking change p: high s: in progress t: refactor/typing/lint t: bugfix labels

onerandomusername added this to the disnake v2.6 milestone

onerandomusername requested a review from shiftinv

July 27, 2022 19:27

onerandomusername mentioned this pull request

chore: rename flag converter file to flag_converter to avoid conflicting with ext.commands flag classes #667

Merged

8 tasks

onerandomusername changed the base branch from master to chore/rename-command-flags

July 27, 2022 19:37

ooliver1 suggested changes

View reviewed changes

disnake/app_commands.py Outdated Show resolved Hide resolved

disnake/app_commands.py Outdated Show resolved Hide resolved

disnake/app_commands.py Outdated Show resolved Hide resolved

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

onerandomusername force-pushed the refactor/app-commands branch from 7540d25 to 2df688b Compare

July 27, 2022 19:42

onerandomusername added the do not merge label

onerandomusername linked an issue

that may be closed by this pull request

Add way to mention slash commands #631

Open

onerandomusername force-pushed the refactor/app-commands branch from a87bd19 to 4ae21f8 Compare

July 28, 2022 06:52

onerandomusername force-pushed the chore/rename-command-flags branch from 083284f to 360fcee Compare

July 28, 2022 07:26

onerandomusername force-pushed the refactor/app-commands branch from 4ae21f8 to 3d1a96d Compare

July 28, 2022 07:26

shiftinv deleted the branch DisnakeDev:master

July 28, 2022 12:53

shiftinv closed this

Member

shiftinv commented Jul 28, 2022

Not sure what happened here, merging #667 resulted in GitHub automatically closing this PR, instead of changing the base branch to master like it should've:

If you delete a head branch after its pull request has been merged, GitHub checks for any open pull requests in the same repository that specify the deleted branch as their base branch. GitHub automatically updates any such pull requests, changing their base branch to the merged pull request's base branch.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-branches#working-with-branches

This worked fine in #616 - in any case, GitHub doesn't allow reopening this PR, looks like you may have to recreate it, sorry :/
(Perhaps due to different source repos? The documentation doesn't appear to mention anything about that though.)

shiftinv reopened this

shiftinv changed the base branch from chore/rename-command-flags to master

July 28, 2022 13:22

Member

shiftinv commented Jul 28, 2022

... should be fixed except for merge conflicts, thanks to https://github.com/github/docs/discussions/18311.

EQUENOS reviewed

View reviewed changes

Member

EQUENOS left a comment

Great job! Here're some comments:

disnake/ext/commands/interaction_bot_base.py

Comment on lines +266 to +258

+                              if disnake.utils.get(
+                                  self._all_app_commands,
+                                  name=slash_command.name,
+                                  type=slash_command.type,
+                                  guild_id=guild_id,
+                              ):
+                                  raise CommandRegistrationError(slash_command.name, guild_id=guild_id)
+                              self._all_app_commands[
+                                  AppCommandMetadata(slash_command.name, guild_id, slash_command.type)
+                              ] = slash_command

Member

EQUENOS Jul 28, 2022

What if we construct a named tuple here and use it as a key to check the existence? Like so:

command_metadata = AppCommandMetadata(slash_command.name, guild_id, slash_command.type)
if command_metadata in self._all_app_commands:
    raise CommandRegistrationError(slash_command.name, guild_id=guild_id)
self._all_app_commands[command_metadata] = slash_command

Same for similar samples below

disnake/ext/commands/interaction_bot_base.py

Comment on lines +416 to 398

+                      command: InvokableSlashCommand = self._all_app_commands.pop(meta, None)  # type: ignore
                       if command is None:
                           return None
                       return command

Member

EQUENOS Jul 28, 2022

Suggested change

      
                    command: InvokableSlashCommand = self._all_app_commands.pop(meta, None)  # type: ignore
          
                    if command is None:
          
                        return None
          
                    return command
          
                    return self._all_app_commands.pop(meta, None)  # type: ignore

disnake/ext/commands/interaction_bot_base.py

Comment on lines +442 to 420

+                      command: InvokableUserCommand = self._all_app_commands.pop(meta, None)  # type: ignore
                       if command is None:
                           return None
                       return command

Member

EQUENOS Jul 28, 2022

Same suggestion as above. Same for similar samples

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

+                      all_commands = self._all_app_commands
+                      for api_command in self._connection._global_application_commands.values():
+                          cmdmeta = disnake.utils.get(all_commands, name=api_command.name, guild_id=None)

Member

EQUENOS Jul 28, 2022

Again, we can speed this up by constructing meta first and then checking if it's in the dict:

cmdmeta = AppCommandMetadata(api_command.name, api_command.type, None)
cmd = all_commands.get(cmdmeta)
if cmd is None:
    continue

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

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

shiftinv force-pushed the refactor/app-commands branch from dd2a89d to 09fcf9b Compare

July 31, 2022 21:58

onerandomusername force-pushed the refactor/app-commands branch 3 times, most recently from 18e5271 to d83abf8 Compare

August 14, 2022 06:57

shiftinv mentioned this pull request

Fix app command copies #678

Merged

8 tasks

Member

shiftinv commented Aug 26, 2022

@onerandomusername Could you resolve the conflicts in this PR?

onerandomusername mentioned this pull request

feat: new command sync options #806

Merged

8 tasks

onerandomusername added 16 commits

October 21, 2022 00:28


          feat: add ID attributes to ApplicationCommand classes

ed76909


          feat: make InvokableCommands also subclasses of ApplicationCommand cl…

8e6cc3c

…asses


          feat: support providing ids to sync commands

a62168a


          fix: if a user-provided command ID is invalid ignore it


          feat: move command sync config to flags

237ac58


          feat: add never_delete and global or guild command syncing options

a625e20


          chore: invert never_delete to allow deletion

a1356e8


          feat: containerize each slash command type

8fca143

resolves disnake#260


          "fix" pyright issues

9230d8e


          fix: sync command ids when bulk updating guild slash commands

fe9d277


          chore: add parents properties

ec79472


          feat: add slash command mentions

e5c619b


          fix some bugs

7cec79e


          chore: remove reassigning imported attributes


          remove removed parameter documentation


          chore: resolve lint and typing issues

7eda7e4

onerandomusername force-pushed the refactor/app-commands branch from 6266281 to 7eda7e4 Compare

October 21, 2022 04:29


          Revert "feat: move command sync config to flags"

e824ed8

This reverts commit 237ac58.

onerandomusername force-pushed the refactor/app-commands branch from b041cdf to 3f94527 Compare

October 21, 2022 04:33


          chore: remove other parts of command sync

onerandomusername force-pushed the refactor/app-commands branch from 3f94527 to 7115195 Compare

October 21, 2022 04:35

onerandomusername modified the milestones: disnake v2.7, disnake v2.8


          Merge branch 'master' into refactor/app-commands

911f124

shiftinv removed this from the disnake v2.8 milestone

EQUENOS reviewed

View reviewed changes

disnake/client.py

+                      res = await self._connection.bulk_overwrite_global_commands(application_commands)
+                      for api_command in res:
+                          cmd = utils.get(application_commands, name=api_command.name)

Member

EQUENOS Mar 1, 2023

It's possible to have 2 app commands of different types (or guilds) with identical names, so this should be modified

EQUENOS reviewed

View reviewed changes

disnake/ext/commands/ctx_menus_core.py

@@ @@ -30,7 +31,7 @@ @@
               __all__ = ("InvokableUserCommand", "InvokableMessageCommand", "user_command", "message_command")
-              class InvokableUserCommand(InvokableApplicationCommand):
+              class InvokableUserCommand(InvokableApplicationCommand, UserCommand):

Member

EQUENOS Mar 1, 2023

What's the point of subclassing UserCommand? The chain of super() calls never reaches it (see InvokableApplicationCommand.__init__) and it doesn't seem to make any typing improvements

Member

shiftinv commented Sep 18, 2023

Since this has become somewhat outdated, and given the number of merge conflicts that have cropped up, I believe this can be closed in favour of #1107 - nonetheless, thanks for the work on this PR c:

shiftinv closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change do not merge p: high s: in progress t: bugfix t: enhancement t: refactor/typing/lint