-
-
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
Changes from 27 commits
6dbe118
b4a10c5
38b72da
79e426f
3ff6628
13848f0
edb32b6
7e8a254
89a210b
f2700a9
624ce4d
2353a82
574235c
0145ecb
62a59ba
21b07a2
62e6b8e
b2173bf
a4aeab9
a79a693
621af5f
b52ded6
6224fb5
63e799d
33dae2e
8a004a1
999b619
332f1ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,7 +64,7 @@ | |
from .hybrid import hybrid_command, hybrid_group, HybridCommand, HybridGroup | ||
|
||
if TYPE_CHECKING: | ||
from typing_extensions import Self | ||
from typing_extensions import Self, Unpack | ||
|
||
import importlib.machinery | ||
|
||
|
@@ -80,12 +80,24 @@ | |
MaybeAwaitableFunc, | ||
) | ||
from .core import Command | ||
from .hybrid import CommandCallback, ContextT, P | ||
from .hybrid import CommandCallback, ContextT, P, _HybridCommandDecoratorKwargs, _HybridGroupDecoratorKwargs | ||
from discord.client import _ClientOptions | ||
from discord.shard import _AutoShardedClientOptions | ||
|
||
_Prefix = Union[Iterable[str], str] | ||
_PrefixCallable = MaybeAwaitableFunc[[BotT, Message], _Prefix] | ||
PrefixType = Union[_Prefix, _PrefixCallable[BotT]] | ||
|
||
class _BotOptions(_ClientOptions, total=False): | ||
owner_id: int | ||
owner_ids: Collection[int] | ||
strip_after_prefix: bool | ||
case_insensitive: bool | ||
|
||
class _AutoShardedBotOptions(_AutoShardedClientOptions, _BotOptions): | ||
... | ||
|
||
|
||
__all__ = ( | ||
'when_mentioned', | ||
'when_mentioned_or', | ||
|
@@ -169,7 +181,7 @@ def __init__( | |
allowed_contexts: app_commands.AppCommandContext = MISSING, | ||
allowed_installs: app_commands.AppInstallationType = MISSING, | ||
intents: discord.Intents, | ||
**options: Any, | ||
**options: Unpack[_BotOptions], | ||
) -> None: | ||
super().__init__(intents=intents, **options) | ||
self.command_prefix: PrefixType[BotT] = command_prefix # type: ignore | ||
|
@@ -281,7 +293,7 @@ def hybrid_command( | |
name: Union[str, app_commands.locale_str] = MISSING, | ||
with_app_command: bool = True, | ||
*args: Any, | ||
**kwargs: Any, | ||
**kwargs: Unpack[_HybridCommandDecoratorKwargs], # type: ignore # name, with_app_command | ||
) -> Callable[[CommandCallback[Any, ContextT, P, T]], HybridCommand[Any, P, T]]: | ||
"""A shortcut decorator that invokes :func:`~discord.ext.commands.hybrid_command` and adds it to | ||
the internal command list via :meth:`add_command`. | ||
|
@@ -293,8 +305,8 @@ def hybrid_command( | |
""" | ||
|
||
def decorator(func: CommandCallback[Any, ContextT, P, T]): | ||
kwargs.setdefault('parent', self) | ||
result = hybrid_command(name=name, *args, with_app_command=with_app_command, **kwargs)(func) | ||
kwargs.setdefault('parent', self) # type: ignore # parent is not for the user to set | ||
result = hybrid_command(name=name, *args, with_app_command=with_app_command, **kwargs)(func) # type: ignore # name, with_app_command | ||
self.add_command(result) | ||
return result | ||
|
||
|
@@ -305,7 +317,7 @@ def hybrid_group( | |
name: Union[str, app_commands.locale_str] = MISSING, | ||
with_app_command: bool = True, | ||
*args: Any, | ||
**kwargs: Any, | ||
**kwargs: Unpack[_HybridGroupDecoratorKwargs], # type: ignore # name, with_app_command | ||
) -> Callable[[CommandCallback[Any, ContextT, P, T]], HybridGroup[Any, P, T]]: | ||
"""A shortcut decorator that invokes :func:`~discord.ext.commands.hybrid_group` and adds it to | ||
the internal command list via :meth:`add_command`. | ||
|
@@ -317,8 +329,8 @@ def hybrid_group( | |
""" | ||
|
||
def decorator(func: CommandCallback[Any, ContextT, P, T]): | ||
kwargs.setdefault('parent', self) | ||
result = hybrid_group(name=name, *args, with_app_command=with_app_command, **kwargs)(func) | ||
kwargs.setdefault('parent', self) # type: ignore # parent is not for the user to set | ||
result = hybrid_group(name=name, *args, with_app_command=with_app_command, **kwargs)(func) # type: ignore # name, with_app_command | ||
self.add_command(result) | ||
return result | ||
|
||
|
@@ -1527,4 +1539,18 @@ class AutoShardedBot(BotBase, discord.AutoShardedClient): | |
.. versionadded:: 2.0 | ||
""" | ||
|
||
pass | ||
if TYPE_CHECKING: | ||
|
||
def __init__( | ||
self, | ||
command_prefix: PrefixType[BotT], | ||
*, | ||
help_command: Optional[HelpCommand] = _default, | ||
tree_cls: Type[app_commands.CommandTree[Any]] = app_commands.CommandTree, | ||
description: Optional[str] = None, | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Same nit as before. |
||
) -> None: | ||
... |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,18 +44,30 @@ | |
Tuple, | ||
TypeVar, | ||
Union, | ||
TypedDict, | ||
) | ||
|
||
from ._types import _BaseCommand, BotT | ||
|
||
if TYPE_CHECKING: | ||
from typing_extensions import Self | ||
from typing_extensions import Self, Unpack | ||
from discord.abc import Snowflake | ||
from discord._types import ClientT | ||
|
||
from .bot import BotBase | ||
from .context import Context | ||
from .core import Command | ||
from .core import Command, _CommandDecoratorKwargs | ||
|
||
class _CogKwargs(TypedDict, total=False): | ||
name: str | ||
group_name: Union[str, app_commands.locale_str] | ||
description: str | ||
group_description: Union[str, app_commands.locale_str] | ||
group_nsfw: bool | ||
group_auto_locale_strings: bool | ||
group_extras: Dict[Any, Any] | ||
command_attrs: _CommandDecoratorKwargs | ||
|
||
|
||
__all__ = ( | ||
'CogMeta', | ||
|
@@ -169,7 +181,7 @@ async def bar(self, ctx): | |
__cog_app_commands__: List[Union[app_commands.Group, app_commands.Command[Any, ..., Any]]] | ||
__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 commentThe 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 commentThe 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. |
||
name, bases, attrs = args | ||
if any(issubclass(base, app_commands.Group) for base in bases): | ||
raise TypeError( | ||
|
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 fromClient
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.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.