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

with_any_role_check updated #145

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

patchwork-systems
Copy link
Contributor

Summary

I've updated the previously purposed HasAnyRole check to be more inline with how the checks work now.

I think the nox pipelines pass for the code we have added.

If this is still not flexible enough as was a previous concern, we'd be happy to update it however necessary.

Checklist

  • I have run nox and all the pipelines have passed.
  • I have made unittests according to the code I have added/modified/deleted.

Related issues

Previous PR with discussion. Extremely out of date with the current version.

tanjun/checks.py Outdated Show resolved Hide resolved
tanjun/checks.py Outdated Show resolved Hide resolved
tanjun/checks.py Show resolved Hide resolved
tanjun/checks.py Outdated Show resolved Hide resolved
tanjun/checks.py Outdated Show resolved Hide resolved
tanjun/checks.py Outdated Show resolved Hide resolved
tanjun/checks.py Outdated Show resolved Hide resolved
tanjun/checks.py Show resolved Hide resolved
tanjun/checks.py Outdated Show resolved Hide resolved
tanjun/checks.py Outdated Show resolved Hide resolved
tanjun/checks.py Outdated Show resolved Hide resolved
Copy link

@davfsa davfsa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

tanjun/checks.py Show resolved Hide resolved
tanjun/checks.py Show resolved Hide resolved
tanjun/checks.py Show resolved Hide resolved
tanjun/checks.py Outdated Show resolved Hide resolved
tanjun/checks.py Outdated Show resolved Hide resolved
tanjun/checks.py Outdated
if not ctx.member:
return self._handle_result(False)

member_roles = ctx.member.get_roles()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this should probably fall back to rest if it can't get roles from the cache/doesn't get any.

tanjun/checks.py Outdated
if not ctx.member:
return self._handle_result(False)

member_roles = ctx.member.get_roles()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably use ctx.cache and filter the roles ourselves instead of relying on ctx.member.get_roles as there's probably cases where ctx.cache won't be the same cache instance as what's attached to ctx.member.app (if ctx.member.app even is cache bound) and it'd be more consistent to rely on what tanjun was initialised with

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We made a few changes in the new commit in line with what you had suggested in the support server.

I am not sure it's the cleanest way to do it, but when we tested it tonight with and without cache enabled it worked!

We are also still thinking about the best way to set required_roles to a property and coerce the provided values into ids. You had mentioned doing that in the __init__ method, but I think we would need async functionality to lookup possible roles to get their id (if provided as a Role.name/str).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You had mentioned doing that in the init method, but I think we would need async functionality to lookup possible roles to get their id (if provided as a Role.name/str).

This isn't quite what I mean; what i meant was more checking all of the roles provided in the init to see if they're all IDs and then setting a flag to indicate whether they're all IDs or not. Since if they're all IDs then a fast route which just directly compares against member.role_ids could be added

Patchwork Collective added 10 commits February 1, 2022 21:20
Removed unused/leftover slots.
Loosened HasAnyRoleCheck and with_any_role_check paramter requirements to a more general type.
Changed HasAnyRoleCheck.required_roles to a set type for better performance.
Simplified hanlder logic.
Reverted internal roles typing from set to list.
Simplified role checking logic.
Updated docs to clarify check locks commands to guilds.
… work. It's not the most elegant solution or the fastes for sure.

Snab had mentioned making `HasAnyRole.required_roles` a property and coercing all the roles to ids for faster comparision. We might add that in later.
@@ -509,6 +511,43 @@ async def __call__(
return self._handle_result((permissions & self._permissions) == self._permissions)


class HasAnyRoleCheck(_Check):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One style thing, since this was originally written doc strings have been added to the check classes and their inits so that would prob be added for this as well

halt_execution: bool = True,
) -> None:
super().__init__(error_message, halt_execution)
self.required_roles = roles
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another style thing, these attributes should be private now (so _required_roles and _ids_only

return self._handle_result(any(map(self._check_roles, member_roles)))

def _check_roles(self, member_role: typing.Union[int, hikari.Role]) -> bool:
if isinstance(member_role, int):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather than instance check here could the different checks be performed in the separate parts of the if not self._ids_only else statement to avoid the instance checks all together (if type checking doesn't quite like this you can just cast but i don't think it should be too strict for equality checks)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants