API clean ups #130
Merged
API clean ups #130
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR cleans up the APIs by streamlining error handling and reducing redundancy across resource and ACL-related operations. Key changes include:
- Updated logging behavior in copy and move operations instead of throwing exceptions when targets already exist.
- Removed multiple overloaded allow/deny (and related) methods from ACL classes.
- Minor syntax adjustments in the README for Groovy code formatting.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| core/src/main/java/dev/vml/es/acm/core/repo/RepoResource.java | Updated error handling in copy and move methods with log messages instead of exceptions. |
| core/src/main/java/dev/vml/es/acm/core/acl/authorizable/AclAuthorizable.java | Removed numerous overloaded methods to simplify the API. |
| core/src/main/java/dev/vml/es/acm/core/acl/Acl.java | Removed multiple overloaded methods to streamline permission operations. |
| README.md | Adjusted Groovy syntax for improved clarity and string interpolation. |
Comments suppressed due to low confidence (5)
core/src/main/java/dev/vml/es/acm/core/repo/RepoResource.java:266
- Changing the behavior from throwing an exception to logging and returning the target on an existing resource may be a breaking change for API consumers. Consider documenting this change clearly or introducing a deprecation phase.
LOG.info("Skipped copying resource from '{}' to '{}' as it already exists", path, target.getPath());
core/src/main/java/dev/vml/es/acm/core/repo/RepoResource.java:305
- The updated move operation now logs and returns the target instead of throwing an exception when the target exists. Ensure that the API documentation reflects this behavior change to avoid unexpected client-side consequences.
LOG.info("Skipped moving resource from '{}' to '{}' as it already exists", path, target.getPath());
core/src/main/java/dev/vml/es/acm/core/acl/authorizable/AclAuthorizable.java:189
- The removal of these overloaded allow (and similarly deny) methods may break existing client code. It is advisable to provide a deprecation phase or update the documentation to notify API consumers of these breaking changes.
return apply(options, true);
core/src/main/java/dev/vml/es/acm/core/acl/Acl.java:585
- Removing the overloaded allow (and corresponding deny) methods here streamlines the API but may cause compatibility issues with existing integrations. Please ensure that API consumers are well informed and corresponding documentation is updated accordingly.
context.getLogger()
README.md:268
- The removal of the safe navigation operator ('?.') in the 'johnDoe.with' call could lead to a NullPointerException if johnDoe is null. Verify that johnDoe is guaranteed to be non-null post-change or reintroduce the null-check if necessary.
johnDoe.with {
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.