Added simple base for a has_any_role decorator check.
#81
Conversation
nxtlo
left a comment
nxtlo
left a comment
There was a problem hiding this comment.
Ain't this suppose to check if the member was human? or None?
tanjun/checks.py
Outdated
| ) -> bool: | ||
|
|
||
| if not ctx.member: | ||
| return _handle_result(False, "You must be a member to use this!", True) |
There was a problem hiding this comment.
This isn't really a good way to handle this, these fields should also be decided based on user input and while halt_execution can just be re-used here you'd likely have to distinguish between the two error messages somehow (possibly by taking them as separate keyword-arguments).
tanjun/checks.py
Outdated
| if not ctx.member: | ||
| return _handle_result(False, "You must be a member to use this!", True) | ||
|
|
||
| member_roles = ctx.member.get_roles() |
There was a problem hiding this comment.
It would probably make sense to fallback to fetching the roles from rest if necessary to make this check more compatible with a cacheless bot (e.g. interaction server).
Plus if we process the roles while initialising the class for this check we could add a hot path for when no role names are provided which avoids getting the role objects all together and rather just checks against ctx.member.role_ids
|
Just got those couple of comments I added which need addressing (sorry if it's a bit much, what you've written so far is good, just need some changes to bring it inline with the other checks) + there's a couple of errors popping up in the CI which need to be solves (for reference you can also run the CI's checks locally pretty easily using nox (similar to tox and more info can be found on it here https://nox.thea.codes/en/stable/, the tasks it has can be found at https://github.com/FasterSpeeding/Tanjun/blob/master/noxfile.py) |
|
We will work on add these changes in today! |
- with_any_roles_check internal logic separated into classes for compatibility - Removed 3.10 only features
|
We made some of the changes requested tonight! The check was transformed into a class to use with Components and everything fits the proper style and nox linting. We started reading up on how to fix the roles issue with the rest client tonight. We'll try to get that fix in tomorrow. We can also apply that fix for the |
How you're handling the member check is fine for the most part right now, just need to ensure that it's error handling is configurable (so use |
|
Might want to rebase this onto master when you next work on it, I've updated the github actions which apply to prs quite a bit since |
|
Instead of trying to rebase and update all of this, we elected to just make a new PR instead: #145 |
3 participants
I just wanted to submit this for an initial review. I'd be happy to make any changes that might be necessary!