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

feat(perm): Support PermissionValidator for different permission check behaviors. #512

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

Conversation

CarmJos
Copy link
Contributor

@CarmJos CarmJos commented Jan 15, 2025

link #429

In this PR, Meta of "Permisisons" changed to Set<Set<String>>, witch makes developers can easily hanle multiple @Permissions annotation, means each @Permission splited to a single Set, and each permission are put into it.

Also, this pr supplied 2 different PermissionValidator, with STRICT(behaviors like before) and PARALLEL(match any one of permissions' set). And the default one is "Strict", which should keep the same behavior as before, and may not affect current users.

Anyway, due developing, we found there's some problems that testPermissionSilent method in different platforms using a static MissingPermissions#checkmethod to test permissions. That makes it difficult to active custom PermissionValidator in root commands' check.
To avoid changes in platforms, you may need to think how to recode it to make it possible to read validators or any other settings of builder supplied in platform manager.

Many thanks for your review!

@huanmeng-qwq huanmeng-qwq mentioned this pull request Jan 15, 2025
@Rollczi
Copy link
Owner

Rollczi commented Jan 18, 2025

Thanks for the update. It seems like we may need to force a change in the behavior of the @Permission annotation, as it’s hard to work around it in a clean way. The "OR" behavior is definitely the one we prefer in this case, so we'll look into adjusting it accordingly. I can take care of the other changes needed.

Thanks again for your input!

@Rollczi
Copy link
Owner

Rollczi commented Jan 18, 2025

YourCraftMC#1

@CarmJos
Copy link
Contributor Author

CarmJos commented Jan 18, 2025

YourCraftMC#1

Merged.

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.

3 participants