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(commands): Fix application command checks' typing #1048

Merged
merged 25 commits into from
Nov 4, 2023

Conversation

elenakrittik
Copy link
Contributor

@elenakrittik elenakrittik commented Jun 13, 2023

Summary

This PR fixes typings related to application commands' checks (their addition/removal) and also implements commands.app_check and commands.app_check_any as an equivalent of commands.check and commands.check_any for application commands as making existing function type hint with application commands is near to impossible.

Fixes #1045

sorry for the .gitignore change

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

@Victorsitou Victorsitou added s: in progress Issue/PR is being worked on t: refactor/typing/lint Refactors, typing changes and/or linting changes labels Jun 14, 2023
@Victorsitou Victorsitou added this to the disnake v2.10 milestone Jun 14, 2023
@elenakrittik elenakrittik marked this pull request as ready for review June 14, 2023 02:40
@elenakrittik
Copy link
Contributor Author

No idea why RTD build fails, but other than that this PR is ready.

@elenakrittik elenakrittik changed the title fix(commands): Renovate (app)command checks' typing fix(commands): Fix application command checks' typing Jun 14, 2023
@onerandomusername
Copy link
Member

No idea why RTD build fails, but other than that this PR is ready.

Retriggering, might just be a quirk.

@shiftinv shiftinv mentioned this pull request Jul 4, 2023
8 tasks
Copy link
Member

@shiftinv shiftinv left a comment

Choose a reason for hiding this comment

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

thanks!
The addition of app_check(_any) is a good idea, I don't see a way to make the existing decorator work with app commands either, especially in situations like @check(lambda inter: ...).

disnake/ext/commands/core.py Show resolved Hide resolved
disnake/ext/commands/core.py Outdated Show resolved Hide resolved
changelog/1045.feature.rst Outdated Show resolved Hide resolved
disnake/ext/commands/base_core.py Outdated Show resolved Hide resolved
disnake/ext/commands/interaction_bot_base.py Show resolved Hide resolved
disnake/ext/commands/interaction_bot_base.py Show resolved Hide resolved
disnake/ext/commands/core.py Outdated Show resolved Hide resolved
disnake/ext/commands/core.py Show resolved Hide resolved
@elenakrittik
Copy link
Contributor Author

Done.

@shiftinv shiftinv added s: needs review Issue/PR is awaiting reviews and removed s: in progress Issue/PR is being worked on labels Sep 5, 2023
Copy link
Member

@shiftinv shiftinv left a comment

Choose a reason for hiding this comment

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

Sorry for the long wait, finally got around to looking at this again.
The check_any decorator typing is still a bit iffy, but after spending some more time going down the rabbit hole of improving decorator typing more generally, it's out of scope for this PR.

disnake/ext/commands/core.py Outdated Show resolved Hide resolved
disnake/ext/commands/core.py Outdated Show resolved Hide resolved
disnake/ext/commands/core.py Show resolved Hide resolved
Copy link
Member

@shiftinv shiftinv left a comment

Choose a reason for hiding this comment

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

lgtm, thanks! Finally getting rid of all those # type: ignores in cogs is a great change.

@shiftinv shiftinv merged commit 59b101f into DisnakeDev:master Nov 4, 2023
27 checks passed
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: refactor/typing/lint Refactors, typing changes and/or linting changes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

A defect in InteractionBotBase typings causes valid code to not type check properly.
4 participants