-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Unpack usage where possible #10189
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
Unpack usage where possible #10189
Conversation
""" | ||
|
||
def __init__(self, *, intents: Intents, **options: Any) -> None: | ||
def __init__(self, *, intents: Intents, **options: Unpack[_ClientOptions]) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, the reason why I didn't do this before for Client
is because as part of the contract that you can inherit from Client
this means that any extra args would cause type errors even though they might use the extra kwargs themselves.
I'm unsure where I really stand on this from a usability POV, and I don't know if it's that annoying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure what you mean as extra kwargs are ignored anyways and you lose the typed kwargs when subclassing and overriding the __init__
to handle the extra args anyways.
class MyClient(discord.Client):
...
# Argument missing for parameter "intents" (expected)
# No parameter named "hello" (expected but ignored anyways)
MyClient(hello="lol")
class ClientWithInit(discord.Client):
# *args = tuple[Unknown, ...], **kwargs = dict[str, Unknown]
def __init__(self, *args, **kwargs) -> None:
# handling extra kwarg
self.foo = kwargs.pop("foo")
super().__init__(*args, **kwargs)
ClientWithInit(foo="bar") # works fine
def __init__(self, *, intents: Intents, **options: Unpack[_ClientOptions]) -> None: | |
def __init__(self, *, intents: Intents, **options: Unpack[_ClientOptions]) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other issue is that it doesn't allow people who inherit to have the same ability, e.g. ClientWithInit(intents=None)
passes despite it not meeting the type signature. I guess it's fine just unfortunate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, there’s unfortunately no good way to make these public. The user can, if they want, import them in a TYPE_CHECKING
block, but that’s probably not good advice to give.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not going to be supported -- and what I'm talking about is an actual supported way of doing this. I guess it has no impact on this PR. The most annoying thing right now is the duplication of all the flags/permissions.
allowed_contexts: app_commands.AppCommandContext = MISSING, | ||
allowed_installs: app_commands.AppInstallationType = MISSING, | ||
intents: discord.Intents, | ||
**kwargs: Unpack[_AutoShardedBotOptions], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same nit as before.
__cog_listeners__: List[Tuple[str, str]] | ||
|
||
def __new__(cls, *args: Any, **kwargs: Any) -> CogMeta: | ||
def __new__(cls, *args: Any, **kwargs: Unpack[_CogKwargs]) -> CogMeta: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this even work from a user's POV?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately not. It does error out when the user has typed the correct name but the wrong type.
__slots__ = () | ||
|
||
def __init__(self, value: int = 0, **kwargs: bool) -> None: | ||
def __init__(self, value: int = 0, **kwargs: Unpack[_IntentsFlagsKwargs]) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another reason why I was not a fan of doing this is because now there are more places to edit when adding a single thing. This is especially egregious with permissions which need to have an added value both in its TYPE_CHECKING
block and now in its Unpack
dictionary.
I suppose it's unavoidable, but honestly it's just really annoying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point, it's a bit annoying to update things in more than one place. But for users, it's kinda nice to have. Also, most people don’t know you can use kwargs to construct flags like this, so showing them helps with that too.
_connection: AutoShardedConnectionState | ||
|
||
def __init__(self, *args: Any, intents: Intents, **kwargs: Any) -> None: | ||
def __init__(self, *args: Any, intents: Intents, **kwargs: Unpack[_AutoShardedClientOptions]) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same nit as Client
and AutoShardedBot
.
discord/permissions.py
Outdated
raise TypeError(f'{key!r} is not a valid permission name.') from None | ||
else: | ||
self._set_flag(flag, value) | ||
self._set_flag(flag, kwvalue) # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this need a type ignore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all values are object
when iterating over a TypedDict, so it throws the following:
Argument of type "object" cannot be assigned to parameter "toggle" of type "bool" in function "_set_flag"
"object" is not assignable to "bool"PylancereportArgumentType
AFAIK this is a not-fixable TypedDict annoyance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's pretty goofy. Add it in the comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Summary
https://canary.discord.com/channels/336642139381301249/1371429669395759195
Tracking
Checklist