GH-886 Sign editor (/sign setline <index> <text> command) #895
Conversation
Took 1 hour 0 minutes
Took 11 minutes
|
Warning Rate limit exceeded@vLuckyyy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThe pull request introduces a comprehensive sign editing feature for a Minecraft plugin. The changes include creating a new package for sign editor functionality, adding translation support for both English and Polish languages, and implementing command classes that allow players to modify sign lines. The implementation involves creating a new command structure with 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignEditCommand.java (2)
31-48: Consider extracting the block distance as a constant.The value 5 in
player.getTargetBlock(null, 5)should be defined as a named constant for better maintainability.+ private static final int SIGN_EDIT_DISTANCE = 5; void execute(@Context Player player, @Arg Integer index, @Join String text) { - Block targetBlock = player.getTargetBlock(null, 5); + Block targetBlock = player.getTargetBlock(null, SIGN_EDIT_DISTANCE);
50-58: Add null check for deserialized text.The deserialized text should be checked for null to prevent potential NPE.
- frontSide.setLine(index, this.miniMessage.deserialize(text).toString()); + String deserializedText = this.miniMessage.deserialize(text).toString(); + if (deserializedText != null) { + frontSide.setLine(index, deserializedText); + }eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java (1)
926-927: Fix Polish description in English translation file.The description comment is in Polish ("Ta sekcja...") but should be in English for consistency.
- @Description({ " ", "# Ta sekcja odpowiada za wiadomości dotyczące edycji tabliczek"}) + @Description({ " ", "# This section is responsible for sign editing messages"})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignEditCommand.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/Translation.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java
🔇 Additional comments (3)
eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignEditCommand.java (1)
17-27: Clean command setup with proper dependency injection!eternalcore-core/src/main/java/com/eternalcode/core/translation/Translation.java (1)
477-483: Well-structured translation interface section!The SignEditSection follows the established pattern and provides all necessary messages.
eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java (1)
945-954: Polish translations are well-implemented!The messages are properly translated and maintain consistency with the English version.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java (2)
935-936: Polish comment in English translation file.The description comment is in Polish. Let's keep it in English for consistency.
- @Description({ " ", "# Ta sekcja odpowiada za wiadomości dotyczące edycji tabliczek"}) + @Description({ " ", "# This section is responsible for sign editing messages"})
941-943: Add descriptions for individual messages.Let's document the placeholders used in these messages to help other developers.
@Contextual public static class SignEditor { + @Description("# Message shown when no sign is found at target location") public Notice noSignFound = Notice.chat("<red>✘ <dark_red>Sign not found!"); + @Description("# Message shown when line index is not between 0-3") public Notice invalidIndex = Notice.chat("<red>✘ <dark_red>Invalid index!"); + @Description({" ", "# {LINE} - Line number (0-3)", "# {TEXT} - New text content"}) public Notice lineSet = Notice.chat("<green>► <white>Line <green>{LINE} <white>set to <green>{TEXT}"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/Translation.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java
- eternalcore-core/src/main/java/com/eternalcode/core/translation/Translation.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (17)
🔇 Additional comments (4)
eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java (3)
957-959: LGTM! Section declaration looks good.The new sign editor section is properly declared and documented.
960-962: LGTM! Annotations are correctly applied.The
@Getterand@Contextualannotations are properly used, consistent with other sections.
963-966: LGTM! Messages are well-structured.The messages follow the plugin's style and include proper placeholders for dynamic content.
eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java (1)
935-944: LGTM! The implementation follows the established patterns.The SignEditor class is well-structured and consistent with other translation sections in the file.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignSideEditorCommand.java (1)
34-34:⚠️ Potential issueAdd null check for target block
Same issue as in
SignEditorCommand- need to check if target block is null.
🧹 Nitpick comments (5)
eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java (1)
937-938: Consider using English for all comments.The comment is in Polish ("Ta sekcja odpowiada za...") while the rest of the file uses English comments.
- @Description({ " ", "# Ta sekcja odpowiada za wiadomości dotyczące edycji tabliczek"}) + @Description({ " ", "# This section is responsible for sign editor messages"})eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignEditorCommand.java (1)
32-32: Consider making the range a constantThe block range of 5 would be clearer as a named constant.
+ private static final int SIGN_EDIT_RANGE = 5; + @Execute(name = "setline") void execute(@Context Player player, @Arg int index, @Join String text) { - Block targetBlock = player.getTargetBlock(null, 5); + Block targetBlock = player.getTargetBlock(null, SIGN_EDIT_RANGE);eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignSideEditorCommand.java (1)
19-22: Consider merging commands using inheritanceThis command shares a lot of code with
SignEditorCommand. Consider creating a base class to avoid duplication.eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/messages/ENSignEditorMessages.java (2)
8-10: Add a quick comment about fluent accessorsConsider adding a brief comment explaining that this class uses fluent-style getters (like
messageInstance.noSignFound()instead ofmessageInstance.getNoSignFound()).
12-14: Make the message fields finalSince these are constant messages, consider making them final:
- public Notice noSignFound = Notice.chat("<red>✘ <dark_red>Sign not found, please look at the sign!"); - public Notice invalidIndex = Notice.chat("<red>✘ <dark_red>Invalid index!"); - public Notice lineSet = Notice.chat("<green>► <white>Line <green>{LINE} <white>set to <green>{TEXT}"); + public final Notice noSignFound = Notice.chat("<red>✘ <dark_red>Sign not found, please look at the sign!"); + public final Notice invalidIndex = Notice.chat("<red>✘ <dark_red>Invalid index!"); + public final Notice lineSet = Notice.chat("<green>► <white>Line <green>{LINE} <white>set to <green>{TEXT}");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignEditorCommand.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignSideEditorCommand.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/messages/ENSignEditorMessages.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/messages/PLSignEditorMessages.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/messages/SignEditorMessages.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/Translation.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- eternalcore-core/src/main/java/com/eternalcode/core/translation/Translation.java
- eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (17)
🔇 Additional comments (7)
eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/messages/PLSignEditorMessages.java (2)
8-11: LGTM! Clean class setup with appropriate annotations.The class is well-structured with Lombok annotations for clean getter generation.
12-14: Messages are clear and user-friendly.The Polish translations are well-formatted with consistent styling using MiniMessage format.
eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java (1)
5-6: LGTM! Appropriate imports added.The imports for sign editor message classes are correctly placed.
eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/messages/SignEditorMessages.java (1)
5-9: Nice and clean interface!The methods are clear and focused on their purpose.
eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignSideEditorCommand.java (1)
34-34: Consider making the range a constantSame suggestion as in
SignEditorCommand- the block range should be a named constant.eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/messages/ENSignEditorMessages.java (2)
1-7: Looks good!The package structure and imports are clean and well-organized.
11-11: Perfect class naming!Clear implementation of the messages interface with good naming convention.
eternalcore-core/src/main/java/com/eternalcode/core/feature/signeditor/SignEditorCommand.java
Outdated
Show resolved
Hide resolved
...ore/src/main/java/com/eternalcode/core/feature/signeditor/messages/ENSignEditorMessages.java
Outdated
Show resolved
Hide resolved
| public class ENSignEditorMessages implements SignEditorMessages { | ||
| public Notice noSignFound = Notice.chat("<red>✘ <dark_red>Sign not found, please look at the sign!"); | ||
| public Notice invalidIndex = Notice.chat("<red>✘ <dark_red>Invalid index!"); | ||
| public Notice lineSet = Notice.chat("<green>► <white>Line <green>{INDEX} <white>set to <green>{TEXT}"); |
There was a problem hiding this comment.
Maybe remove formatting for text if the text added to sign should be formatted
There was a problem hiding this comment.
I mean that formatting used to set text to ex. Bold could overlap with formatting of message so the text in Notice would be bold and green instead of just bold
No description provided.