From f4c5d37c8fe265b5d63d0f250e5165e9b8d8b16f Mon Sep 17 00:00:00 2001 From: Rapptz Date: Sun, 1 May 2022 13:39:44 -0400 Subject: [PATCH] [commands] Rework Cog + Group inheritance to requite GroupCog This is an attempt to fix the MRO issues present in the current implementation. The previous implementation of using both Cog and app_commands.Group in the inheritance chain caused issues with things such as walk_commands due to it potentially shadowing the app_commands version of the call. In this particular case it's better to use composition instead of inheritance to avoid these bugs entirely. Especially as more things are added that could conflict with each other. --- discord/ext/commands/bot.py | 6 +- discord/ext/commands/cog.py | 159 ++++++++++++++++++++++--------- docs/ext/commands/api.rst | 9 ++ tests/test_app_commands_group.py | 40 ++++---- 4 files changed, 152 insertions(+), 62 deletions(-) diff --git a/discord/ext/commands/bot.py b/discord/ext/commands/bot.py index 0fd1a03ed..ed65ebda5 100644 --- a/discord/ext/commands/bot.py +++ b/discord/ext/commands/bot.py @@ -754,8 +754,8 @@ class BotBase(GroupMixin[None]): raise discord.ClientException(f'Cog named {cog_name!r} already loaded') await self.remove_cog(cog_name, guild=guild, guilds=guilds) - if isinstance(cog, app_commands.Group): - self.__tree.add_command(cog, override=override, guild=guild, guilds=guilds) + if cog.__cog_app_commands_group__: + self.__tree.add_command(cog.__cog_app_commands_group__, override=override, guild=guild, guilds=guilds) cog = await cog._inject(self, override=override, guild=guild, guilds=guilds) self.__cogs[cog_name] = cog @@ -841,7 +841,7 @@ class BotBase(GroupMixin[None]): help_command.cog = None guild_ids = _retrieve_guild_ids(cog, guild, guilds) - if isinstance(cog, app_commands.Group): + if cog.__cog_app_commands_group__: if guild_ids is None: self.__tree.remove_command(name) else: diff --git a/discord/ext/commands/cog.py b/discord/ext/commands/cog.py index 5a8ddcb1e..6fff5a2f5 100644 --- a/discord/ext/commands/cog.py +++ b/discord/ext/commands/cog.py @@ -28,7 +28,7 @@ import discord from discord import app_commands from discord.utils import maybe_coroutine -from typing import Any, Callable, Dict, Generator, Iterable, List, Optional, TYPE_CHECKING, Tuple, TypeVar, Union +from typing import Any, Callable, ClassVar, Dict, Generator, Iterable, List, Optional, TYPE_CHECKING, Tuple, TypeVar, Union from ._types import _BaseCommand, BotT @@ -43,6 +43,7 @@ if TYPE_CHECKING: __all__ = ( 'CogMeta', 'Cog', + 'GroupCog', ) FuncT = TypeVar('FuncT', bound=Callable[..., Any]) @@ -108,36 +109,58 @@ class CogMeta(type): @commands.command(hidden=False) async def bar(self, ctx): pass # hidden -> False + + group_name: :class:`str` + The group name of a cog. This is only applicable for :class:`GroupCog` instances. + By default, it's the same value as :attr:`name`. + + .. versionadded:: 2.0 + group_description: :class:`str` + The group description of a cog. This is only applicable for :class:`GroupCog` instances. + By default, it's the same value as :attr:`description`. + + .. versionadded:: 2.0 """ __cog_name__: str + __cog_description__: str + __cog_group_name__: str + __cog_group_description__: str __cog_settings__: Dict[str, Any] __cog_commands__: List[Command[Any, ..., Any]] - __cog_is_app_commands_group__: bool __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) -> Self: name, bases, attrs = args - attrs['__cog_name__'] = kwargs.get('name', name) - attrs['__cog_settings__'] = kwargs.pop('command_attrs', {}) - is_parent = any(issubclass(base, app_commands.Group) for base in bases) - attrs['__cog_is_app_commands_group__'] = is_parent + if any(issubclass(base, app_commands.Group) for base in bases): + raise TypeError( + 'Cannot inherit from app_commands.Group with commands.Cog, consider using commands.GroupCog instead' + ) - description = kwargs.get('description', None) + # If name='...' is given but not group_name='...' then name='...' is used for both. + # If neither is given then cog name is the class name but group name is kebab case + try: + cog_name = kwargs.pop('name') + except KeyError: + cog_name = name + try: + group_name = kwargs.pop('group_name') + except KeyError: + group_name = app_commands.commands._to_kebab_case(name) + else: + group_name = kwargs.pop('group_name', cog_name) + + attrs['__cog_settings__'] = kwargs.pop('command_attrs', {}) + attrs['__cog_name__'] = cog_name + attrs['__cog_group_name__'] = group_name + + description = kwargs.pop('description', None) if description is None: description = inspect.cleandoc(attrs.get('__doc__', '')) - attrs['__cog_description__'] = description - if is_parent: - attrs['__discord_app_commands_skip_init_binding__'] = True - # This is hacky, but it signals the Group not to process this info. - # It's overridden later. - attrs['__discord_app_commands_group_children__'] = True - else: - # Remove the extraneous keyword arguments we're using - kwargs.pop('name', None) - kwargs.pop('description', None) + attrs['__cog_description__'] = description + attrs['__cog_group_description__'] = kwargs.pop('group_description', description or '\u2026') commands = {} cog_app_commands = {} @@ -180,12 +203,6 @@ class CogMeta(type): new_cls.__cog_commands__ = list(commands.values()) # this will be copied in Cog.__new__ new_cls.__cog_app_commands__ = list(cog_app_commands.values()) - if is_parent: - # Prefill the app commands for the Group as well.. - # The type checker doesn't like runtime attribute modification and this one's - # optional so it can't be cheesed. - new_cls.__discord_app_commands_group_children__ = new_cls.__cog_app_commands__ # type: ignore - listeners_as_list = [] for listener in listeners.values(): for listener_name in listener.__cog_listener_names__: @@ -221,10 +238,15 @@ class Cog(metaclass=CogMeta): """ __cog_name__: str + __cog_description__: str + __cog_group_name__: str + __cog_group_description__: str __cog_settings__: Dict[str, Any] __cog_commands__: List[Command[Self, ..., Any]] __cog_app_commands__: List[Union[app_commands.Group, app_commands.Command[Self, ..., Any]]] __cog_listeners__: List[Tuple[str, str]] + __cog_is_app_commands_group__: ClassVar[bool] = False + __cog_app_commands_group__: Optional[app_commands.Group] def __new__(cls, *args: Any, **kwargs: Any) -> Self: # For issue 426, we need to store a copy of the command objects @@ -242,6 +264,20 @@ class Cog(metaclass=CogMeta): # Register the application commands children: List[Union[app_commands.Group, app_commands.Command[Self, ..., Any]]] = [] + if cls.__cog_is_app_commands_group__: + group = app_commands.Group( + name=cls.__cog_group_name__, + description=cls.__cog_group_description__, + parent=None, + guild_ids=getattr(cls, '__discord_app_commands_default_guilds__', []), + guild_only=getattr(cls, '__discord_app_commands_guild_only__', False), + default_permissions=getattr(cls, '__discord_app_commands_default_permissions__', None), + ) + else: + group = None + + self.__cog_app_commands_group__ = group + # Update the Command instances dynamically as well for command in self.__cog_commands__: setattr(self, command.callback.__name__, command) @@ -253,42 +289,43 @@ class Cog(metaclass=CogMeta): # Update our parent's reference to our self parent.remove_command(command.name) # type: ignore parent.add_command(command) # type: ignore - elif cls.__cog_is_app_commands_group__: + elif self.__cog_app_commands_group__: if hasattr(command, '__commands_is_hybrid__') and command.parent is None: # In both of these, the type checker does not see the app_command attribute even though it exists command.app_command = command.app_command._copy_with(parent=self, binding=self) # type: ignore children.append(command.app_command) # type: ignore for command in cls.__cog_app_commands__: - copy = command._copy_with( - # Type checker doesn't understand this type of narrowing. - # Not even with TypeGuard somehow. - parent=self if cls.__cog_is_app_commands_group__ else None, # type: ignore - binding=self, - ) + copy = command._copy_with(parent=self.__cog_app_commands_group__, binding=self) + + # Update set bindings + if copy._attr: + setattr(self, copy._attr, copy) children.append(copy) self.__cog_app_commands__ = children - if cls.__cog_is_app_commands_group__: - # Dynamic attribute setting - self.__discord_app_commands_group_children__ = children # type: ignore - # Enforce this to work even if someone forgets __init__ - self.module = cls.__module__ # type: ignore + if self.__cog_app_commands_group__: + self.__cog_app_commands_group__.module = cls.__module__ + mapping = {cmd.name: cmd for cmd in children} + if len(mapping) > 25: + raise TypeError('maximum number of application command children exceeded') + + self.__cog_app_commands_group__._children = mapping # type: ignore # Variance issue return self def get_commands(self) -> List[Command[Self, ..., Any]]: - r""" + r"""Returns the commands that are defined inside this cog. + + This does *not* include :class:`discord.app_commands.Command` or :class:`discord.app_commands.Group` + instances. + Returns -------- List[:class:`.Command`] A :class:`list` of :class:`.Command`\s that are - defined inside this cog. - - .. note:: - - This does not include subcommands. + defined inside this cog, not including subcommands. """ return [c for c in self.__cog_commands__ if c.parent is None] @@ -322,6 +359,14 @@ class Cog(metaclass=CogMeta): if isinstance(command, GroupMixin): yield from command.walk_commands() + @property + def app_command_group(self) -> Optional[app_commands.Group]: + """Optional[:class:`discord.app_commands.Group`]: Returns the associated group with this cog. + + This is only available if inheriting from :class:`GroupCog`. + """ + return self.__cog_app_commands_group__ + def get_listeners(self) -> List[Tuple[str, Callable[..., Any]]]: """Returns a :class:`list` of (name, function) listener pairs that are defined in this cog. @@ -531,7 +576,7 @@ class Cog(metaclass=CogMeta): bot.add_listener(getattr(self, method_name), name) # Only do this if these are "top level" commands - if not cls.__cog_is_app_commands_group__: + if not self.__cog_app_commands_group__: for command in self.__cog_app_commands__: # This is already atomic bot.tree.add_command(command, override=override, guild=guild, guilds=guilds) @@ -546,7 +591,7 @@ class Cog(metaclass=CogMeta): if command.parent is None: bot.remove_command(command.name) - if not cls.__cog_is_app_commands_group__: + if not self.__cog_app_commands_group__: for command in self.__cog_app_commands__: guild_ids = guild_ids or command._guild_ids if guild_ids is None: @@ -568,3 +613,31 @@ class Cog(metaclass=CogMeta): await maybe_coroutine(self.cog_unload) except Exception: pass + + +class GroupCog(Cog): + """Represents a cog that also doubles as a parent :class:`discord.app_commands.Group` for + the application commands defined within it. + + This inherits from :class:`Cog` and the options in :class:`CogMeta` also apply to this. + See the :class:`Cog` documentation for methods. + + Decorators such as :func:`~discord.app_commands.guild_only`, :func:`~discord.app_commands.guilds`, + and :func:`~discord.app_commands.default_permissions` will apply to the group if used on top of the + cog. + + For example: + + .. code-block:: python3 + + from discord import app_commands + from discord.ext import commands + + @app_commands.guild_only() + class MyCog(commands.GroupCog, group_name='my-cog'): + pass + + .. versionadded:: 2.0 + """ + + __cog_is_app_commands_group__: ClassVar[bool] = True diff --git a/docs/ext/commands/api.rst b/docs/ext/commands/api.rst index 048d93896..bf3706adf 100644 --- a/docs/ext/commands/api.rst +++ b/docs/ext/commands/api.rst @@ -238,6 +238,15 @@ Cog .. autoclass:: discord.ext.commands.Cog :members: +GroupCog +~~~~~~~~~ + +.. attributetable:: discord.ext.commands.GroupCog + +.. autoclass:: discord.ext.commands.GroupCog + :members: + + CogMeta ~~~~~~~~ diff --git a/tests/test_app_commands_group.py b/tests/test_app_commands_group.py index 827c38d5d..756f7c48f 100644 --- a/tests/test_app_commands_group.py +++ b/tests/test_app_commands_group.py @@ -260,20 +260,22 @@ def test_cog_with_group_subclass_with_group_subclass(): def test_cog_group_with_commands(): - class MyCog(commands.Cog, app_commands.Group): + class MyCog(commands.GroupCog): @app_commands.command() async def my_command(self, interaction: discord.Interaction) -> None: ... cog = MyCog() - assert MyCog.__discord_app_commands_group_children__[0].parent is not cog + assert MyCog.__cog_app_commands__[0].parent is not cog + assert MyCog.__cog_app_commands__[0].parent is not cog.__cog_app_commands_group__ assert cog.my_command is not MyCog.my_command - assert cog.parent is None - assert cog.my_command.parent is cog + assert cog.__cog_app_commands_group__ is not None + assert cog.__cog_app_commands_group__.parent is None + assert cog.my_command.parent is cog.__cog_app_commands_group__ def test_cog_group_with_group(): - class MyCog(commands.Cog, app_commands.Group): + class MyCog(commands.GroupCog): sub_group = app_commands.Group(name='mysubgroup', description='My sub-group') @sub_group.command() @@ -281,11 +283,13 @@ def test_cog_group_with_group(): ... cog = MyCog() - assert MyCog.__discord_app_commands_group_children__[0].parent is not cog + assert MyCog.__cog_app_commands__[0].parent is not cog + assert MyCog.__cog_app_commands__[0].parent is not cog.__cog_app_commands_group__ assert cog.sub_group is not MyCog.sub_group assert cog.my_command is not MyCog.my_command - assert cog.parent is None - assert cog.sub_group.parent is cog + assert cog.__cog_app_commands_group__ is not None + assert cog.__cog_app_commands_group__.parent is None + assert cog.sub_group.parent is cog.__cog_app_commands_group__ assert cog.my_command.parent is cog.sub_group @@ -295,7 +299,7 @@ def test_cog_group_with_subclass_group(): async def my_command(self, interaction: discord.Interaction) -> None: ... - class MyCog(commands.Cog, app_commands.Group): + class MyCog(commands.GroupCog): sub_group = MyGroup() @sub_group.command() @@ -303,14 +307,16 @@ def test_cog_group_with_subclass_group(): ... cog = MyCog() - assert MyCog.__discord_app_commands_group_children__[0].parent is not cog + assert MyCog.__cog_app_commands__[0].parent is not cog + assert MyCog.__cog_app_commands__[0].parent is not cog.__cog_app_commands_group__ assert MyGroup.__discord_app_commands_group_children__[0].parent is not cog.sub_group assert cog.sub_group is not MyCog.sub_group assert cog.sub_group.my_command is not MyGroup.my_command assert cog.my_cog_command is not MyCog.my_cog_command assert not hasattr(cog.sub_group, 'my_cog_command') - assert cog.parent is None - assert cog.sub_group.parent is cog + assert cog.__cog_app_commands_group__ is not None + assert cog.__cog_app_commands_group__.parent is None + assert cog.sub_group.parent is cog.__cog_app_commands_group__ assert cog.sub_group.my_command.parent is cog.sub_group assert cog.my_cog_command.parent is cog.sub_group assert cog.my_cog_command.binding is cog @@ -325,7 +331,7 @@ def test_cog_group_with_subclassed_subclass_group(): class MySubclassedGroup(MyGroup, name='mygroup'): ... - class MyCog(commands.Cog, app_commands.Group): + class MyCog(commands.GroupCog): sub_group = MySubclassedGroup() @sub_group.command() @@ -333,7 +339,8 @@ def test_cog_group_with_subclassed_subclass_group(): ... cog = MyCog() - assert MyCog.__discord_app_commands_group_children__[0].parent is not cog + assert MyCog.__cog_app_commands__[0].parent is not cog + assert MyCog.__cog_app_commands__[0].parent is not cog.__cog_app_commands_group__ assert MyGroup.__discord_app_commands_group_children__[0].parent is not cog.sub_group assert MySubclassedGroup.__discord_app_commands_group_children__[0].parent is not cog.sub_group assert cog.sub_group is not MyCog.sub_group @@ -341,8 +348,9 @@ def test_cog_group_with_subclassed_subclass_group(): assert cog.sub_group.my_command is not MySubclassedGroup.my_command assert cog.my_cog_command is not MyCog.my_cog_command assert not hasattr(cog.sub_group, 'my_cog_command') - assert cog.parent is None - assert cog.sub_group.parent is cog + assert cog.__cog_app_commands_group__ is not None + assert cog.__cog_app_commands_group__.parent is None + assert cog.sub_group.parent is cog.__cog_app_commands_group__ assert cog.sub_group.my_command.parent is cog.sub_group assert cog.my_cog_command.parent is cog.sub_group assert cog.my_cog_command.binding is cog