fix: 🧑💻 Add kwargs explicitly for app-command decorators#3119
fix: 🧑💻 Add kwargs explicitly for app-command decorators#3119ToothyDev wants to merge 8 commits intoPycord-Development:masterfrom
Conversation
|
Thanks for opening this pull request! This pull request can be checked-out with: git fetch origin pull/3119/head:pr-3119
git checkout pr-3119This pull request can be installed with: pip install git+https://github.com/Pycord-Development/pycord@refs/pull/3119/head |
ToothyDev
requested review from
EmmmaTech,
JustaSqu1d,
Lulalaby and
plun1331
Paillat-dev
left a comment
Paillat-dev
left a comment
There was a problem hiding this comment.
Should do the trick for now
Paillat-dev
left a comment
Paillat-dev
left a comment
There was a problem hiding this comment.
Should do the trick for now
Signed-off-by: ToothyDev <55001472+ToothyDev@users.noreply.github.com>
|
Why are none of these kwargs only? This will look confusing to the "beginning" users. |
|
opinions @Paillat-dev @JustaSqu1d |
|
@Soheab is right. Where the original only accepted **kwargs these should be kwarg-only as well. |
plun1331
left a comment
plun1331
left a comment
There was a problem hiding this comment.
whatever the other person said about kwargs
Co-authored-by: plun1331 <plun1331@gmail.com> Signed-off-by: ToothyDev <55001472+ToothyDev@users.noreply.github.com>
| nsfw: bool = False, | ||
| options: list[Option] | None = MISSING, | ||
| parent: SlashCommandGroup | None = MISSING, | ||
| **kwargs: Any, |
There was a problem hiding this comment.
Since all applicable kwargs are explicitly typed now, shouldn't we remove this?
There was a problem hiding this comment.
We could but it would break for people that were passing non-existent params previously. Which is kinda their fault but we also did accept **kwargs. I am fine either ways personally.
There was a problem hiding this comment.
we should remove it def in future. but for rn. lets do a vote in discord
There was a problem hiding this comment.
I think we should remove it in this pull request while we're at it tbh. We did allow **kwargs but non-existent params had no effect anyway.
There was a problem hiding this comment.
We could disallow it at type checking using overloads or if TYPE_CHECKING: ...
There was a problem hiding this comment.
I agree w/soheab. And maybe have a deprecation warning or runtime warning
There was a problem hiding this comment.
I think overloads will be messy here for all these different functions, I'd be on Doru's side. This isn't breaking correct code.
There was a problem hiding this comment.
What about **kwargs: Never? Type checkers will reject unknown kwargs at call sites, and it won't cause any runtime crashes for existing callers (nevertheless, removing **kwargs completely seems the best).
There was a problem hiding this comment.
Note that guild_only is missing from all of these, and while it is deprecated, its deprecation is entirely separate from any potential deprecation/breakage we might have here. It should still be allowed where it was before. We haven't specified any removal date for guild_only=value in its own deprecation notice.
8 participants
Summary
This explicitly adds typed kwargs to the
slash_command,user_commandandmessage_commanddecorators as well as to the basicapplication_commanddecorator.Information
examples, ...).
Checklist
type: ignorecomments were used, a comment is also left explaining why.