search

python-discord / bot

The community bot for the Python Discord community
https://pythondiscord.com
MIT License
1.34k stars 664 forks source link

remove with_roles decorator and replace with has_any_role #217

Closed lemonsaurus closed 3 years ago

lemonsaurus commented 5 years ago

Initially, when we wrote the with_roles decorator, discord.py's own has_any_role did not support IDs as input. Around a month ago (in this commit), this was changed.

That means, there's no longer any point in us having our own decorator for this. Our decorator should be replaced with has_any_role. See https://discordpy.readthedocs.io/en/rewrite/ext/commands/api.html#discord.ext.commands.has_any_role for documentation.

lemonsaurus commented 5 years ago

Okay, so, I no longer think we should do this.

The discord.py framework does not provide a negated version of this. We have both with_role and without_role, and we use them both a lot. Changing our with_role only would mean that we'd have has_any_role and without_role decorators that no longer matched, and would be imported from different places. This is uglier than what we've currently got.

Closing this ticket, since it hasn't seen any discussion anyway.

GhostofGoes commented 5 years ago

Would suggesting the addition of without_role functionality to discord.py be worthwhile?

lemonsaurus commented 5 years ago

I don't see why not.

lemonsaurus commented 5 years ago

Okay, so after a chat with @scragly, here's an approach that would solve all of the concerns raised above:

We import the new has_any_role straight into our decorators.py, then we remove our with_role decorator and rewrite our without_role to just use has_any_role but negate the result. That way the user can import both from the same place, they both have logically similar names, and you get all the speed benefits of the new decorator (which is O(1) instead of O(n)).

kosayoda commented 4 years ago

From #236: Some cogs use the cog_check method to add a check to every command in the cog. The cog_check method is the predicate itself, which poses a problem if we were to use has_any_role, which is decorator check.

One solution would be to nab has_any_role from discordpy, place it in decorators.py, and split the predicate and check.

That way we get the goodness and the usefulness of having both a decorator and a predicate for cog_checks. I can work on this if we're moving forward with this idea.

scragly commented 4 years ago

@kosayoda I think that's a good solution. If you get a chance to do it, that'll be great. I've assigned you to the ticket.

MarkKoz commented 4 years ago

No progress for 4 months; unassigning. This is now open for anyone to work on.