disnake icon indicating copy to clipboard operation
disnake copied to clipboard

fix: Another blind spot of command sync algorithm #1108

Open koshakkkq opened this issue 2 years ago • 2 comments

Summary

Fix of Another blind spot of command sync algorithm #1108

Checklist

  • [x] If code changes were made, then they have been tested
    • [ ] I have updated the documentation to reflect the changes
    • [x] I have formatted the code properly by running pdm lint
    • [x] I have type-checked the code by running pdm pyright
  • [x] 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, ...) We need to check ids from both guild_cmds and self._connection._guild_application_commands.keys(), otherwise on deletion, we cant find any ids in local commands, and cycle doesnt procceed any values

koshakkkq avatar Oct 29 '23 14:10 koshakkkq

Thanks for the PR - the two comments above are just for code style, in terms of functionality there are more quirks here that need to be ironed out, though.

Manually registering commands is supported through Client.create_guild_command, which adds the command to _connection._guild_application_commands but doesn't affect Bot.all_slash_commands, i.e. the command isn't part of guild_cmds in this PR. As a result, with this change, any manually registered commands get removed on the next sync as well, which would be breaking. (This already happens when mixing manual and automatic registration within the same guild, but this PR would expand that behavior to situations where guilds only have manually registered commands.)

I'm unsure if this is solvable without a more general refactor, and #1107 touches the same areas of the code as this PR, so it might be wise to see how that turns out first.

shiftinv avatar Nov 03 '23 13:11 shiftinv

Also as @shiftinv said, this PR should probably wait for #1107 because it interferes with this one.

EQUENOS avatar Nov 03 '23 16:11 EQUENOS