refactor app command sync and other fixes by onerandomusername · Pull Request #666 · DisnakeDev/disnake

onerandomusername · 2022-07-27T19:27:22Z

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, ...)

shiftinv · 2022-07-28T13:17:56Z

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 · 2022-07-28T13:25:48Z

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

EQUENOS

Great job! Here're some comments:

EQUENOS · 2022-07-28T18:19:49Z

+                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


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

EQUENOS · 2022-07-28T18:22:57Z

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


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

EQUENOS · 2022-07-28T18:23:53Z

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


Same suggestion as above. Same for similar samples

EQUENOS · 2022-07-28T18:33:27Z

+
+        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)


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

shiftinv · 2022-08-26T18:50:58Z

@onerandomusername Could you resolve the conflicts in this PR?

…asses

resolves disnake#260

This reverts commit 237ac58.

EQUENOS · 2023-03-01T13:41:47Z

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


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

EQUENOS · 2023-03-01T13:48:52Z



-class InvokableUserCommand(InvokableApplicationCommand):
+class InvokableUserCommand(InvokableApplicationCommand, UserCommand):


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

shiftinv · 2023-09-18T13:10:27Z

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:

onerandomusername added t: enhancement New feature breaking change Includes breaking changes to code/packaging p: high High priority s: in progress Issue/PR is being worked on t: refactor/typing/lint Refactors, typing changes and/or linting changes t: bugfix labels Jul 27, 2022

onerandomusername added this to the disnake v2.6 milestone Jul 27, 2022

onerandomusername requested a review from shiftinv July 27, 2022 19:27

onerandomusername mentioned this pull request Jul 27, 2022

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 Jul 27, 2022

View reviewed changes

Comment thread disnake/app_commands.py Outdated

Comment thread disnake/app_commands.py Outdated

Comment thread disnake/app_commands.py Outdated

Comment thread disnake/ext/commands/slash_core.py Outdated

onerandomusername force-pushed the refactor/app-commands branch from 7540d25 to 2df688b Compare July 27, 2022 19:42

onerandomusername added the do not merge Don't merge. Don't. label Jul 28, 2022

onerandomusername linked an issue Jul 28, 2022 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 Jul 28, 2022

shiftinv reopened this Jul 28, 2022

shiftinv changed the base branch from chore/rename-command-flags to master July 28, 2022 13:22

EQUENOS reviewed Jul 28, 2022

View reviewed changes

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 Aug 25, 2022

Fix app command copies #678

Merged

8 tasks

onerandomusername mentioned this pull request Oct 17, 2022

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

8595828

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

6468099

remove removed parameter documentation

9695975

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

7115195

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 Oct 24, 2022

Merge branch 'master' into refactor/app-commands

911f124

shiftinv removed this from the disnake v2.8 milestone Feb 5, 2023

EQUENOS reviewed Mar 1, 2023

View reviewed changes

shiftinv closed this Sep 18, 2023



		class InvokableUserCommand(InvokableApplicationCommand):
		class InvokableUserCommand(InvokableApplicationCommand, UserCommand):

Uh oh!

Conversation

onerandomusername commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shiftinv commented Jul 28, 2022

Uh oh!

shiftinv commented Jul 28, 2022

Uh oh!

EQUENOS left a comment

Choose a reason for hiding this comment

Uh oh!

EQUENOS Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

EQUENOS Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

EQUENOS Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

EQUENOS Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shiftinv commented Aug 26, 2022

Uh oh!

EQUENOS Mar 1, 2023

Choose a reason for hiding this comment

Uh oh!

EQUENOS Mar 1, 2023

Choose a reason for hiding this comment

Uh oh!

shiftinv commented Sep 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

onerandomusername commented Jul 27, 2022 •

edited

Loading