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

Base Cog Class #2151

Closed
Tobotimus opened this issue Sep 26, 2018 · 5 comments
Closed

Base Cog Class #2151

Tobotimus opened this issue Sep 26, 2018 · 5 comments
Labels
Type: Informational Aims to provide information or facilitate discussion.
Milestone

Comments

@Tobotimus
Copy link
Member

Tobotimus commented Sep 26, 2018

This issue aims to provide information about why I plan on introducing a base cog class to Red. Some people have already noticed its inclusion in #2149, and rightly suspect it's a breaking change they will need to adjust for going forwards.

We've never really needed a base cog class up until now, because we tend to describe the settings and properties of particular cogs in a proxy-like style, rather than attach these settings and properties directly to the cog object. We haven't needed many settings pertaining to specific cogs, so this usage has served us fine. However, #2149 proposes using a different approach - modifying and retrieving properties of a cog during runtime through a direct API to the cog object, similar to how Command objects are used. In fact, the Permissions cog draws some new similarities between how we treat cogs and Command objects.

As always with significant structural changes like these, I imagine we will find many usages going forward for this base cog object which greatly simplify some problems, which would be otherwise quite complex to solve. It's an incredibly simple change for the cog creator to incorporate, whilst potentially adding lots of freedom for us developers to add new functionality to all cogs.

@Tobotimus Tobotimus added V3 Status: In Progress There's a PR open to deal with this, or an org member is working on this internally Type: Informational Aims to provide information or facilitate discussion. labels Sep 26, 2018
@Tobotimus Tobotimus added this to the RC 1 milestone Sep 26, 2018
@Tobotimus
Copy link
Member Author

Tobotimus commented Sep 26, 2018

Now, as for semantic details: the new base cog class will be located at redbot.core.commands.Cog; so incorporating this change won't even involve an extra import.

As a transitional measure, you can do this to prepare for the new base cog class:

BaseCog = getattr(commands, "Cog", object)

class MyCog(BaseCog):
    ...

You may call super().__init__() if you'd like, but if you don't, it will be done for you in bot.add_cog anyway (Disregard this sentence if you don't understand).

The following attributes as added in #2149 will be reserved for cogs, so please don't have any attributes or methods with these names, or things will break!

  • requires
  • all_commands
  • allow_for
  • deny_to
  • clear_rule_for
  • set_default_rule
  • reevaluate_rules_for

@Redjumpman
Copy link
Member

Other than making the permissions cog play better with cogs, what other benefits (either now or in the future) can cog creators expect from inheriting this base class?

@useruser3
Copy link

What other functions will the base class Have? One function I can see it having is allowing a master config file that can be used for many cogs and not tied to a specific one ?

Tobotimus added a commit that referenced this issue Oct 1, 2018
API changes:
- Cogs must now inherit from `commands.Cog` (see #2151 for discussion and more details)
- All functions which are not decorators in the `redbot.core.checks` module are now deprecated in favour of their counterparts in `redbot.core.utils.mod`. This is to make this module more consistent and end the confusing naming convention.
- `redbot.core.checks.check_overrides` function is now gone, overrideable checks can now be created with the `@commands.permissions_check` decorator
- Command, Group, Cog and Context have some new attributes and methods, but they are for internal use so shouldn't concern cog creators (unless they're making a permissions cog!).
- `__permissions_check_before` and `__permissions_check_after` have been replaced:  A cog method named `__permissions_hook` will be evaluated as permissions hooks in the same way `__permissions_check_before` previously was. Permissions hooks can also be added/removed/verified through the new `*_permissions_hook()` methods on the bot object, and they will be verified even when permissions is unloaded.
- New utility method `redbot.core.utils.chat_formatting.humanize_list`
- New dependency [`schema`](https://github.com/keleshev/schema)

User-facing changes:
- When a `@bot_has_permissions` check fails, the bot will respond saying what permissions were actually missing.
- All YAML-related `[p]permissions` subcommands now reside under the `[p]permissions acl` sub-group (tbh I still think the whole cog has too many top-level commands)
- The YAML schema for these commands has been changed
- A rule cannot be set as allow and deny at the same time (previously this would just default to allow)

Documentation:
- New documentation for `redbot.core.commands.requires` and `redbot.core.checks` modules
- Renewed documentation for the permissions cog
- `sphinx.ext.doctest` is now enabled

Note: standard discord.py checks will still behave exactly the same way, in fact they are checked before `Requires` is looked at, so they are not overrideable. 

Signed-off-by: Toby Harradine <tobyharradine@gmail.com>
@Tobotimus Tobotimus removed the Status: In Progress There's a PR open to deal with this, or an org member is working on this internally label Oct 1, 2018
@Tobotimus
Copy link
Member Author

Tobotimus commented Oct 2, 2018

@Redjumpman @useruser3 I don't plan on polluting this class with methods and attributes or try to provide a plethora of functionality to it, however I'm sure in the future it will make solving complicated problems quite simple for us maintainers. Just look at how redbot.core.commands turned out - initially we only needed that for i18n on command help, but looking back, it's hard to know what we would have done without it.

For now, you get the all_commands property, which returns a dict mapping command names to commands which come from that cog 😃 I don't have any specific plans otherwise. There's a possibility that in the future it'll allow the use of check decorators like @checks.is_owner() on cogs to apply that check to every command from that cog.

@Tobotimus
Copy link
Member Author

Closing now that the base cog class has been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Informational Aims to provide information or facilitate discussion.
Projects
None yet
Development

No branches or pull requests

3 participants