jsedding commented on code in PR #45: URL: https://github.com/apache/sling-org-apache-sling-jcr-repoinit/pull/45#discussion_r1387259771
########## src/main/java/org/apache/sling/jcr/repoinit/impl/AclVisitor.java: ########## Review Comment: I did this refactoring so that `SetAclPrincipals` and `SetAclPaths` share the same code-path and to remove code-duplication. For symmetry, and because just removing the "set ACL" part from the method `setAcl` seemed strange, I did the same for `RemoveAcePrincipals` and `RemoveAcePaths`. The final reason, why I didn't want a `setAcl` method in this class is, that it clashes with `AclUtil#setAcl`, which I found confusing. The method `setRepositoryAcl` was a copy & paste of `setAcl`, omitting only the `paths` argument, which was set to `null` in the removed `AclUtil#setRepositoryAcl`. Removing this in favour of explicitly injecting the `:repository` path, seemed much clearer to me. Finally, the `require` method was actually a no-op, because `AclLine#getProperty()` [never returns `null`](https://github.com/apache/sling-org-apache-sling-repoinit-parser/blob/ea340191058fefe84ba9134f8025f71a69295a14/src/main/java/org/apache/sling/repoinit/parser/operations/AclLine.java#L64-L67). If you insist, I can try to revert these changes in favour of a minimal change-set approach. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
