Clarify some parts of the PermissionService documentation #1387
Conversation
|
@zml2008 Please review |
| * <p>Note: This data should be persisted, so plugins that add permissions to this subject | ||
| * must take care to not override permissions already set or modified. It is also | ||
| * recommended to use {@link Subject#getTransientSubjectData()} where possible to | ||
| * avoid persisting unnecessary data. |
There was a problem hiding this comment.
I'm not so sure about the persisting part.
If you modify defaults then its customized and no longer a default. IMO its better to specify a user group that inherits from the default group and overwrite things there.
In addition to that on one hand you force the permission plugin to persist the data, but on the other hand you ask the plugin owner to set transient data only.
Missing </p>tag.
There was a problem hiding this comment.
I agree with you - however, the current intended behaviour is that defaults are persisted.
(at least, thats what I'm assuming, judging from the responses I've had from blood, and from looking at the PEX implementation. I'm all for saying "nope, defaults should be transient", however this PR was about improving documentation, not changing intended behaviours.)
There's missing </p> tags throughout PermissionService. You'll notice they don't appear in the existing JavaDocs either, so I just followed the same convention.
There was a problem hiding this comment.
Yes, the intention is that persistent defaults and transient defaults are exposed.
Generally, the persistent defaults are primarily set by the user and transient defaults are primarily set by plugins, but the permissions API exposes both.
There was a problem hiding this comment.
Yep - that's fine, it just wasn't documented.
There was a problem hiding this comment.
mk good, it would probably be good to say more about the purposes of persistent vs transient (and make sure to update the javadocs in SubjectCollection as well)
There was a problem hiding this comment.
That's already in my commit, kinda. There's an explanation of transient data on the #getTransientSubjectData method.
201386e#diff-10cc1565e2d1c6e37bae54aabd176204R101
I don't think you really need to say more than that.
| * | ||
| * @param contexts The set of contexts that represents the subject's current environment | ||
| * @param permission The permission string | ||
| * @return True if permission is granted | ||
| */ | ||
| boolean hasPermission(Set<Context> contexts, String permission); | ||
| default boolean hasPermission(Set<Context> contexts, String permission) { |
There was a problem hiding this comment.
This should also mention the "format" permission strings are supposed to follow.
(See PermissionDescription for more details)
And it should mention that there are no wildcard.* permissions.
The same applies for getPermissionValue
There was a problem hiding this comment.
Plugins wouldn't normally check if users had wildcard permissions, so I don't think those methods are the correct place to document that.
I added a reminder that implementations should consider "wildcards" in those checks. For the actual explanation, they can read the description in the main Subject class documentation.
|
Any update on this @zml2008? |
This PR attempts to clarify some of the ambiguities in PermissionService, as raised in #1383.
As far as I know, the added javadocs do not conflict with existing intended behaviours, rather just clarifies them.
Possibly relevant: some of the issues were also briefly discussed here: https://forums.spongepowered.org/t/luckperms-an-advanced-permissions-system/14274/118