Extract prefix to static variable in notifications. #50
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several changes to the Possibly related PRs
Suggested labels
Suggested reviewers
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: 2
🧹 Outside diff range and nitpick comments (2)
eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/messages/MessageConfig.java (1)
9-11: Consider making the prefix finalSince this is a constant value that defines your message styling, it would be good to mark it as
finalto prevent accidental changes.- public static String messagesPrefix = + public static final String messagesPrefix =eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/messages/MessageAdminSubSection.java (1)
13-30: Fix inconsistent spacing after prefixSome messages have extra spaces after the prefix while others don't. Let's make them consistent.
Here's a simpler version:
- Notice.chat(messagesPrefix + " <white>Removed <gradient:#00FFA2:#34AE00>{AMOUNT}</gradient> from " + Notice.chat(messagesPrefix + "<white>Removed <gradient:#00FFA2:#34AE00>{AMOUNT}</gradient> from " - Notice.chat(messagesPrefix + " <white><gradient:#00FFA2:#34AE00>{PLAYER}</gradient>'s balance is " + Notice.chat(messagesPrefix + "<white><gradient:#00FFA2:#34AE00>{PLAYER}</gradient>'s balance is "
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- eternaleconomy-core/build.gradle.kts (1 hunks)
- eternaleconomy-core/src/main/java/com/eternalcode/economy/command/cooldown/CommandCooldownConfig.java (1 hunks)
- eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/messages/MessageAdminSubSection.java (1 hunks)
- eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/messages/MessageConfig.java (1 hunks)
- eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/messages/MessagesPlayerSubSection.java (1 hunks)
🧰 Additional context used
🔇 Additional comments (8)
eternaleconomy-core/src/main/java/com/eternalcode/economy/command/cooldown/CommandCooldownConfig.java (1)
3-3: Nice work on standardizing the message prefix!The changes consistently implement the new message prefix pattern, making the codebase more maintainable.
Also applies to: 10-10, 18-18
eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/messages/MessageConfig.java (2)
13-17: Nice work on consistent message formatting! 👍The messages look clean and well-formatted. The white text on all user-facing messages will ensure good readability.
24-25: Good use of gradient for permission displayThe permission message looks great with the gradient highlighting. This will help it stand out to users.
eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/messages/MessageAdminSubSection.java (1)
7-8: LGTM!The static import for MessageConfig is properly used.
eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/messages/MessagesPlayerSubSection.java (1)
6-7: LGTM! Good job centralizing the prefix.The static import helps keep the code clean while reusing the shared prefix.
eternaleconomy-core/build.gradle.kts (3)
81-83: Nice addition of Vault plugin download configuration!This ensures the test environment matches the production dependency version.
Line range hint
89-89: Good job on the clear jar naming convention!Adding the version to the jar name will make it easier to identify different builds.
Line range hint
81-89: Verify if these changes belong in this PR.The PR title mentions extracting prefix to static variable in notifications, but these build changes seem unrelated. Could you clarify if these changes should be in a separate PR?
| Notice.chat(messagesPrefix + "Correct usage:"); | ||
| public Notice correctUsageHeader = Notice.chat("<dark_gray>➤</dark_gray> &fCorrect usage:"); | ||
| public Notice correctUsageEntry = Notice.chat("<dark_gray>➤</dark_gray> &f{USAGE}"); |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
You're right - the styling needs to be unified
Looking at the code, there are indeed inconsistent styles:
- The main prefix uses a gradient-styled "ECONOMY" with an arrow
- Some messages use this full prefix
- Others use just a simple arrow
The review suggestion to unify the style is valid, and it would be better to stick with the main prefix style for consistency.
🔗 Analysis chain
Consider unifying the styling approach
The usage-related notices mix different styles - some use the prefix while others use a simple arrow. It might be clearer to stick to one approach.
Consider updating all usage messages to use the prefix:
- public Notice correctUsageHeader = Notice.chat("<dark_gray>➤</dark_gray> &fCorrect usage:");
- public Notice correctUsageEntry = Notice.chat("<dark_gray>➤</dark_gray> &f{USAGE}");
+ public Notice correctUsageHeader = Notice.chat(messagesPrefix + "Correct usage:");
+ public Notice correctUsageEntry = Notice.chat(messagesPrefix + "{USAGE}");Let me check the actual usage patterns in these files to make a more informed decision.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are other inconsistent styling patterns
rg -l "dark_gray>➤"
Length of output: 213
Script:
#!/bin/bash
# Check the context of arrow usage in both files
rg "dark_gray>➤" -B 2 -A 2
Length of output: 2510
| public Notice added = | ||
| Notice.chat(messagesPrefix + "<white>Added <gradient:#00FFA2:#34AE00>{AMOUNT}</gradient>" | ||
| + "to your account.</white>"); | ||
| public Notice removed = | ||
| Notice.chat(messagesPrefix + " <white>Removed <gradient:#00FFA2:#34AE00>{AMOUNT}</gradient>" | ||
| + "from your account.</white>"); | ||
| public Notice set = | ||
| Notice.chat(messagesPrefix + "<white>Set your balance to <gradient:#00FFA2:#34AE00>{AMOUNT}</gradient>.</white>"); | ||
| public Notice reset = | ||
| Notice.chat(messagesPrefix + "<white>Your balance was reset.</white>"); | ||
| public Notice balance = | ||
| Notice.chat(messagesPrefix + " <white>Your balance is <gradient:#00FFA2:#34AE00>{BALANCE}</gradient>.</white>"); | ||
| public Notice balanceOther = | ||
| Notice.chat("<b><gradient:#00FFA2:#34AE00>ECONOMY</gradient></b> <dark_gray>➤</dark_gray>" | ||
| + " <white><gradient:#00FFA2:#34AE00>{PLAYER}</gradient>'s balance is <gradient:#00FFA2:#34AE00>{BALANCE}</gradient>.</white>"); | ||
| public Notice insufficientBalance = Notice.chat("<b><gradient:#00FFA2:#34AE00>ECONOMY</gradient></b> " | ||
| + "<dark_gray>➤</dark_gray> <white>Insufficient funds," | ||
| Notice.chat(messagesPrefix + " <white><gradient:#00FFA2:#34AE00>{PLAYER}</gradient>'s balance is" | ||
| + "<gradient:#00FFA2:#34AE00>{BALANCE}</gradient>.</white>"); | ||
| public Notice insufficientBalance = Notice.chat(messagesPrefix + "<white>Insufficient funds," | ||
| + " you are missing <gradient:#00FFA2:#34AE00> {MISSING_BALANCE}</gradient>.</white>"); | ||
| public Notice transferSuccess = Notice.chat("<b><gradient:#00FFA2:#34AE00>ECONOMY</gradient></b> <dark_gray" | ||
| + ">➤</dark_gray> <white>Successfully transferred <gradient:#00FFA2:#34AE00>{AMOUNT}</gradient> to " | ||
| + "<gradient:#00FFA2:#34AE00>{PLAYER}</gradient>.</white>"); | ||
| public Notice transferReceived = Notice.chat("<b><gradient:#00FFA2:#34AE00>ECONOMY</gradient></b> " | ||
| + "<dark_gray>➤</dark_gray> <white>Received <gradient:#00FFA2:#34AE00>{AMOUNT}</gradient> from " | ||
| + "<gradient:#00FFA2:#34AE00>{PLAYER}</gradient>.</white>"); | ||
| public Notice transferLimit = Notice.chat("<b><gradient:#00FFA2:#34AE00>ECONOMY</gradient></b> <dark_gray>➤</dark_gray>" | ||
| + " <white>Transaction limit is <gradient:#00FFA2:#34AE00>{LIMIT}</gradient>.</white>"); | ||
| public Notice transferSuccess = | ||
| Notice.chat(messagesPrefix + "<white>Successfully transferred <gradient:#00FFA2:#34AE00>{AMOUNT}</gradient> to " | ||
| + "<gradient:#00FFA2:#34AE00>{PLAYER}</gradient>.</white>"); | ||
| public Notice transferReceived = | ||
| Notice.chat(messagesPrefix + "<white>Received <gradient:#00FFA2:#34AE00>{AMOUNT}</gradient> from " | ||
| + "<gradient:#00FFA2:#34AE00>{PLAYER}</gradient>.</white>"); | ||
| public Notice transferLimit = | ||
| Notice.chat(messagesPrefix + "<white>Transaction limit is <gradient:#00FFA2:#34AE00>{LIMIT}</gradient>.</white>"); |
There was a problem hiding this comment.
Fix inconsistent spacing after prefix.
Some messages have an extra space after the prefix while others don't. Let's make this consistent.
Here's a simple fix for the spacing issues:
- public Notice removed =
- Notice.chat(messagesPrefix + " <white>Removed <gradient:#00FFA2:#34AE00>{AMOUNT}</gradient>"
+ public Notice removed =
+ Notice.chat(messagesPrefix + "<white>Removed <gradient:#00FFA2:#34AE00>{AMOUNT}</gradient>"
- public Notice balance =
- Notice.chat(messagesPrefix + " <white>Your balance is <gradient:#00FFA2:#34AE00>{BALANCE}</gradient>.</white>");
+ public Notice balance =
+ Notice.chat(messagesPrefix + "<white>Your balance is <gradient:#00FFA2:#34AE00>{BALANCE}</gradient>.</white>");
- public Notice balanceOther =
- Notice.chat(messagesPrefix + " <white><gradient:#00FFA2:#34AE00>{PLAYER}</gradient>'s balance is"
+ public Notice balanceOther =
+ Notice.chat(messagesPrefix + "<white><gradient:#00FFA2:#34AE00>{PLAYER}</gradient>'s balance is"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public Notice added = | |
| Notice.chat(messagesPrefix + "<white>Added <gradient:#00FFA2:#34AE00>{AMOUNT}</gradient>" | |
| + "to your account.</white>"); | |
| public Notice removed = | |
| Notice.chat(messagesPrefix + " <white>Removed <gradient:#00FFA2:#34AE00>{AMOUNT}</gradient>" | |
| + "from your account.</white>"); | |
| public Notice set = | |
| Notice.chat(messagesPrefix + "<white>Set your balance to <gradient:#00FFA2:#34AE00>{AMOUNT}</gradient>.</white>"); | |
| public Notice reset = | |
| Notice.chat(messagesPrefix + "<white>Your balance was reset.</white>"); | |
| public Notice balance = | |
| Notice.chat(messagesPrefix + " <white>Your balance is <gradient:#00FFA2:#34AE00>{BALANCE}</gradient>.</white>"); | |
| public Notice balanceOther = | |
| Notice.chat("<b><gradient:#00FFA2:#34AE00>ECONOMY</gradient></b> <dark_gray>➤</dark_gray>" | |
| + " <white><gradient:#00FFA2:#34AE00>{PLAYER}</gradient>'s balance is <gradient:#00FFA2:#34AE00>{BALANCE}</gradient>.</white>"); | |
| public Notice insufficientBalance = Notice.chat("<b><gradient:#00FFA2:#34AE00>ECONOMY</gradient></b> " | |
| + "<dark_gray>➤</dark_gray> <white>Insufficient funds," | |
| Notice.chat(messagesPrefix + " <white><gradient:#00FFA2:#34AE00>{PLAYER}</gradient>'s balance is" | |
| + "<gradient:#00FFA2:#34AE00>{BALANCE}</gradient>.</white>"); | |
| public Notice insufficientBalance = Notice.chat(messagesPrefix + "<white>Insufficient funds," | |
| + " you are missing <gradient:#00FFA2:#34AE00> {MISSING_BALANCE}</gradient>.</white>"); | |
| public Notice transferSuccess = Notice.chat("<b><gradient:#00FFA2:#34AE00>ECONOMY</gradient></b> <dark_gray" | |
| + ">➤</dark_gray> <white>Successfully transferred <gradient:#00FFA2:#34AE00>{AMOUNT}</gradient> to " | |
| + "<gradient:#00FFA2:#34AE00>{PLAYER}</gradient>.</white>"); | |
| public Notice transferReceived = Notice.chat("<b><gradient:#00FFA2:#34AE00>ECONOMY</gradient></b> " | |
| + "<dark_gray>➤</dark_gray> <white>Received <gradient:#00FFA2:#34AE00>{AMOUNT}</gradient> from " | |
| + "<gradient:#00FFA2:#34AE00>{PLAYER}</gradient>.</white>"); | |
| public Notice transferLimit = Notice.chat("<b><gradient:#00FFA2:#34AE00>ECONOMY</gradient></b> <dark_gray>➤</dark_gray>" | |
| + " <white>Transaction limit is <gradient:#00FFA2:#34AE00>{LIMIT}</gradient>.</white>"); | |
| public Notice transferSuccess = | |
| Notice.chat(messagesPrefix + "<white>Successfully transferred <gradient:#00FFA2:#34AE00>{AMOUNT}</gradient> to " | |
| + "<gradient:#00FFA2:#34AE00>{PLAYER}</gradient>.</white>"); | |
| public Notice transferReceived = | |
| Notice.chat(messagesPrefix + "<white>Received <gradient:#00FFA2:#34AE00>{AMOUNT}</gradient> from " | |
| + "<gradient:#00FFA2:#34AE00>{PLAYER}</gradient>.</white>"); | |
| public Notice transferLimit = | |
| Notice.chat(messagesPrefix + "<white>Transaction limit is <gradient:#00FFA2:#34AE00>{LIMIT}</gradient>.</white>"); | |
| public Notice added = | |
| Notice.chat(messagesPrefix + "<white>Added <gradient:#00FFA2:#34AE00>{AMOUNT}</gradient>" | |
| + "to your account.</white>"); | |
| public Notice removed = | |
| Notice.chat(messagesPrefix + "<white>Removed <gradient:#00FFA2:#34AE00>{AMOUNT}</gradient>" | |
| + "from your account.</white>"); | |
| public Notice set = | |
| Notice.chat(messagesPrefix + "<white>Set your balance to <gradient:#00FFA2:#34AE00>{AMOUNT}</gradient>.</white>"); | |
| public Notice reset = | |
| Notice.chat(messagesPrefix + "<white>Your balance was reset.</white>"); | |
| public Notice balance = | |
| Notice.chat(messagesPrefix + "<white>Your balance is <gradient:#00FFA2:#34AE00>{BALANCE}</gradient>.</white>"); | |
| public Notice balanceOther = | |
| Notice.chat(messagesPrefix + "<white><gradient:#00FFA2:#34AE00>{PLAYER}</gradient>'s balance is" | |
| + "<gradient:#00FFA2:#34AE00>{BALANCE}</gradient>.</white>"); | |
| public Notice insufficientBalance = Notice.chat(messagesPrefix + "<white>Insufficient funds," | |
| + " you are missing <gradient:#00FFA2:#34AE00> {MISSING_BALANCE}</gradient>.</white>"); | |
| public Notice transferSuccess = | |
| Notice.chat(messagesPrefix + "<white>Successfully transferred <gradient:#00FFA2:#34AE00>{AMOUNT}</gradient> to " | |
| + "<gradient:#00FFA2:#34AE00>{PLAYER}</gradient>.</white>"); | |
| public Notice transferReceived = | |
| Notice.chat(messagesPrefix + "<white>Received <gradient:#00FFA2:#34AE00>{AMOUNT}</gradient> from " | |
| + "<gradient:#00FFA2:#34AE00>{PLAYER}</gradient>.</white>"); | |
| public Notice transferLimit = | |
| Notice.chat(messagesPrefix + "<white>Transaction limit is <gradient:#00FFA2:#34AE00>{LIMIT}</gradient>.</white>"); |
| public static String messagesPrefix = | ||
| "<b><gradient:#00FFA2:#34AE00>ECONOMY</gradient></b> <dark_gray>➤</dark_gray> "; |
| import com.eternalcode.multification.notice.Notice; | ||
| import eu.okaeri.configs.OkaeriConfig; | ||
| import eu.okaeri.configs.annotation.Comment; | ||
|
|
||
| import java.time.Duration; | ||
|
|
||
| import static com.eternalcode.economy.config.implementation.messages.MessageConfig.*; |
There was a problem hiding this comment.
We usually don't use * in imports. Try to apply MessageConfig via constructor in main class
| public class CommandCooldownConfig extends OkaeriConfig { | ||
| @Comment("Duration of the cooldown (e.g. 5s, 10m, 1h)") | ||
| public Duration duration = Duration.ofSeconds(5); | ||
| @Comment("Permission for admins to bypass the cooldown") | ||
| public String bypassPermission = "eternaleconomy.player.pay.bypass"; | ||
| public Notice message = Notice.builder() | ||
| .chat("<b><gradient:#00FFA2:#34AE00>ECONOMY</gradient></b> <dark_gray>➤</dark_gray> " | ||
| + "<white>You must wait <gradient:#00FFA2:#34AE00>{TIME}</gradient> before using /pay again.") | ||
| .chat(messagesPrefix + "<white>You must wait <gradient:#00FFA2:#34AE00>{TIME}</gradient> before using /pay again.") |
There was a problem hiding this comment.
if this import is needed only for this, maybe try package private and access the variable directly or via method. @Rollczi please think if there is a better way to do it
There was a problem hiding this comment.
It should be done on the principle of internal placeholder, for example {PREFIX} can be used anywhere in the messages, this will allow a little more flexibility, currently you add everywhere, what if someone will have one message set to actionbar and the other to normal chat? I want to have a prefix in the chat and not in the actionbar, then he won't have a choice because you will add a prefix permanently.
There was a problem hiding this comment.
the solution used is based on incorrect assumptions. You can't expect that the prefix will magically be appended to the configuration, to understand this see what the generated config looks like and what happens if you change the prefix in file and reload it.
Tldr: this doesn't work
No description provided.