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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

koshakkkq
Copy link

@koshakkkq koshakkkq commented Oct 29, 2023

Summary

Fix of Another blind spot of command sync algorithm #1108

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, ...)
    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 koshakkkq changed the title Fix of Another blind spot of command sync algorithm #1108 fix(issue) of Another blind spot of command sync algorithm #1108 Oct 29, 2023
@koshakkkq koshakkkq changed the title fix(issue) of Another blind spot of command sync algorithm #1108 fix: Another blind spot of command sync algorithm #1108 Oct 29, 2023
@shiftinv shiftinv added s: needs review Issue/PR is awaiting reviews t: bugfix labels Oct 29, 2023
Comment on lines +833 to +835
guild_ids_to_check = set(guild_cmds.keys())
for guild_id in self._connection._guild_application_commands.keys():
guild_ids_to_check.add(guild_id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified a bit:

Suggested change
guild_ids_to_check = set(guild_cmds.keys())
for guild_id in self._connection._guild_application_commands.keys():
guild_ids_to_check.add(guild_id)
guild_ids_to_check = guild_cmds.keys() | self._connection._guild_application_commands.keys()

current_guild_cmds = self._connection._guild_application_commands.get(guild_id, {})
cmds = guild_cmds.get(guild_id, {})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cmds = guild_cmds.get(guild_id, {})
cmds = guild_cmds.get(guild_id, [])

@shiftinv
Copy link
Member

shiftinv commented Nov 3, 2023

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.

@@ -0,0 +1 @@
Make message_command check both local and synchronized guild_ids
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR fixes multiple things, not only the message_command decorator. I think it'd be better to say which bug has been fixed.

@EQUENOS
Copy link
Member

EQUENOS commented Nov 3, 2023

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

@shiftinv shiftinv linked an issue Nov 3, 2023 that may be closed by this pull request
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s: needs review Issue/PR is awaiting reviews t: bugfix
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

Another blind spot of command sync algorithm
3 participants