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

Remove unnecessary instance check in autocomplete #1643

Merged
merged 2 commits into from
Sep 23, 2022

Conversation

DefiDebauchery
Copy link
Contributor

Summary

Fixes #1630

The autocomplete hooks recently had typechecking applied, which would only pass if a command was specifically a Command. Currently, sub-commands are an instance of SlashCommandGroup. Because of this, the check would fail, and autocomplete Options in subcommands would fail to execute.

Note that ApplicationCommand also encompasses Bridged commands and Context (Message/User) commands. I'm happy to update this PR to change typechecking for (Command, SlashCommandGroup) if needed, but this is better than having subcommand autocomplete being broken.

Information

  • 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, typehinting, examples, ...).

Checklist

  • I have searched the open pull requests for duplicates.
  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • If type: ignore comments were used, a comment is also left explaining why.

@Lulalaby Lulalaby enabled auto-merge (squash) September 22, 2022 15:23
@Dorukyum
Copy link
Member

Maybe we should instead prevent SlashCommandGroups from being passed here

@Lulalaby
Copy link
Member

Maybe we should instead prevent SlashCommandGroups from being passed here

Would be another good approach
@DefiDebauchery what you think

@DefiDebauchery
Copy link
Contributor Author

DefiDebauchery commented Sep 22, 2022

I suppose that depends on whether subcommands are intended to be SlashCommandGroups as they currently are. IIRC there's some limitation about being able to pull subcommands out into a unique Command (but I could be way off here)

But the issue is that autocomplete in subcommands are completely broken in the currently-published version of Pycord. This PR essentially just reverts the typecheck change that happened in this release.

Because of that, I feel that fixing this aspect should take precedence, while the larger-scope task of correcting types is ongoing (assuming it does require some dedicated work).

@Dorukyum
Copy link
Member

Subcommands are SlashCommand objects, they are just stored in SlashCommandGroup.subcommands

I think, while we're at it, we should fix the issue from the root

@Lulalaby Lulalaby disabled auto-merge September 22, 2022 16:27
@DefiDebauchery
Copy link
Contributor Author

Of course. No argument about fixing issues at a fundamental level.

However, I don't think other users of subcommands should be left out of a working version while that takes place. It's currently broken in production.

@Lulalaby
Copy link
Member

@Dorukyum please lets merge this as temporary fix
@BobDotCom please make a rc of that

@Lulalaby Lulalaby enabled auto-merge (squash) September 22, 2022 20:11
@Lulalaby Lulalaby merged commit 5026759 into Pycord-Development:master Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autocomplete does not work for slash commands in groups
3 participants