Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract prefix to static variable in notifications. #50

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

VerniQ
Copy link
Contributor

@VerniQ VerniQ commented Oct 22, 2024

No description provided.

@VerniQ VerniQ linked an issue Oct 22, 2024 that may be closed by this pull request
Copy link

coderabbitai bot commented Oct 22, 2024

Walkthrough

The pull request introduces several changes to the eternaleconomy-core project, primarily affecting the build.gradle.kts file and various Java classes related to message handling. A new downloadPlugins configuration is added to the tasks.runServer section to specify a GitHub repository for the Vault plugin, version "1.7.3". The tasks.shadowJar section is updated to set a new output filename format that includes the project version. In the Java classes, message handling is enhanced by introducing a messagesPrefix variable in the MessageConfig class, which standardizes message formatting across different Notice instances. This update is reflected in classes like CommandCooldownConfig, MessageAdminSubSection, and MessagesPlayerSubSection, where existing message strings are modified to include the new prefix, improving consistency and maintainability.

Possibly related PRs

Suggested labels

🆕 feature

Suggested reviewers

  • CitralFlo
  • Kamicjusz
  • Rollczi
  • vLuckyyy

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5eee01c and b4fcb85.

📒 Files selected for processing (2)
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/EconomyBukkitPlugin.java (1 hunks)
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/EconomyReloadCommand.java (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/EconomyBukkitPlugin.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: build
🔇 Additional comments (4)
eternaleconomy-core/src/main/java/com/eternalcode/economy/EconomyReloadCommand.java (4)

4-4: Nice addition of the MessageConfig import!

The import is well-placed and matches the new functionality.


20-21: Good job updating the notice format!

The addition of the {PREFIX} placeholder helps standardize message formatting while keeping the nice gradient styling.


25-35: Clean constructor update!

The MessageConfig addition follows the existing pattern nicely. Good job making the field final for immutability.


47-47: Perfect placeholder addition!

The PREFIX placeholder is well-integrated into the existing chain of operations.

✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 final

Since this is a constant value that defines your message styling, it would be good to mark it as final to 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 prefix

Some 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

📥 Commits

Files that changed from the base of the PR and between 5bc160a and 3839e1d.

📒 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 display

The 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?

Comment on lines 20 to 22
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}");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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

Comment on lines 10 to 34
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>");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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>");

Comment on lines 10 to 11
public static String messagesPrefix =
"<b><gradient:#00FFA2:#34AE00>ECONOMY</gradient></b> <dark_gray>➤</dark_gray> ";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove static

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.*;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@vLuckyyy vLuckyyy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@Rollczi Rollczi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@VerniQ VerniQ requested a review from vLuckyyy February 8, 2025 22:26
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
eternaleconomy-core/src/main/java/com/eternalcode/economy/command/player/MoneyTransferCommand.java (1)

50-50: Consistent prefix usage across all messages!

Great job adding the prefix placeholder to all notification messages. This ensures a uniform look for all player communications.

One small suggestion - consider extracting the prefix placeholder key to a constant:

+private static final String PREFIX_PLACEHOLDER = "{PREFIX}";

This would make future updates easier if the placeholder format needs to change.

Also applies to: 61-61, 74-74, 82-82

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3839e1d and 5eee01c.

📒 Files selected for processing (17)
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/EconomyBukkitPlugin.java (1 hunks)
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/EconomyReloadCommand.java (3 hunks)
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/command/admin/AdminAddCommand.java (3 hunks)
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/command/admin/AdminBalanceCommand.java (3 hunks)
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/command/admin/AdminRemoveCommand.java (4 hunks)
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/command/admin/AdminResetCommand.java (3 hunks)
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/command/admin/AdminSetCommand.java (3 hunks)
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/command/cooldown/CommandCooldownConfig.java (1 hunks)
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/command/cooldown/CommandCooldownMessage.java (3 hunks)
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/command/handler/InvalidUsageHandlerImpl.java (4 hunks)
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/command/handler/MissingPermissionHandlerImpl.java (3 hunks)
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/command/message/InvalidBigDecimalMessage.java (3 hunks)
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/command/player/MoneyBalanceCommand.java (3 hunks)
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/command/player/MoneyTransferCommand.java (5 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)
🚧 Files skipped from review as they are similar to previous changes (3)
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/command/cooldown/CommandCooldownConfig.java
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/messages/MessageConfig.java
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/messages/MessagesPlayerSubSection.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: build
🔇 Additional comments (49)
eternaleconomy-core/src/main/java/com/eternalcode/economy/command/handler/MissingPermissionHandlerImpl.java (4)

3-3: Looks good!

The new import for MessageConfig is correctly added.


14-14: Looks good!

The new field is properly declared as final and private.


16-22: Looks good!

The constructor is nicely updated to include the new messageConfig parameter.


35-35: Looks good!

The prefix placeholder is correctly added to match the new messaging system.

eternaleconomy-core/src/main/java/com/eternalcode/economy/command/admin/AdminBalanceCommand.java (4)

5-5: Looking good! Import added correctly.

The MessageConfig import is properly placed with other imports.


21-21: Nice addition of the messageConfig field!

The field is properly marked as final and follows the same pattern as other fields.


23-31: Constructor updated nicely!

The constructor parameters are well-organized and the initialization looks good.


39-39: Good use of the new prefix!

The PREFIX placeholder is properly added to match the standardization effort.

eternaleconomy-core/src/main/java/com/eternalcode/economy/command/player/MoneyBalanceCommand.java (4)

5-5: Looking good! Import added for the new MessageConfig.

The new import aligns with the changes in the class.


21-21: Nice addition of the messageConfig field!

The field is correctly marked as final and private.


23-31: Constructor updated nicely with the new parameter.

The constructor is well-organized and properly initializes all fields.


38-38: Good job adding the prefix placeholder consistently!

The PREFIX placeholder is added in both execute methods, maintaining consistency.

Also applies to: 50-50

eternaleconomy-core/src/main/java/com/eternalcode/economy/command/player/MoneyTransferCommand.java (2)

8-8: Nice addition of the message config!

The new MessageConfig will help keep message prefixes consistent across the plugin.

Also applies to: 27-27


33-34: Constructor update looks good!

The MessageConfig is properly initialized in the constructor.

Also applies to: 40-40

eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/messages/MessageAdminSubSection.java (2)

7-8: Nice work on organizing imports!

The static import from MessageConfig is a clean way to access the prefix constant.


12-30: Great job on standardizing the messages!

All messages now consistently use the prefix and maintain a clean, uniform style.

eternaleconomy-core/src/main/java/com/eternalcode/economy/command/message/InvalidBigDecimalMessage.java (4)

3-3: Looks good!

The new import is correctly added and matches the new functionality.


15-15: Perfect field declaration!

The new field follows the same pattern as the existing one.


17-20: Constructor looks great!

The updated constructor properly handles both the old and new parameters.


29-29: Nice placeholder addition!

The new prefix placeholder matches the existing style and helps standardize messages.

eternaleconomy-core/src/main/java/com/eternalcode/economy/command/cooldown/CommandCooldownMessage.java (4)

4-4: Looks good!

The new import is needed for the message prefix feature.


19-19: Nice and clean!

The new field matches the style of other fields.


21-29: Well structured!

The constructor is nicely formatted and properly initializes the new field.


44-44: Perfect addition!

The prefix placeholder follows the same style as other placeholders.

eternaleconomy-core/src/main/java/com/eternalcode/economy/EconomyBukkitPlugin.java (4)

109-109: Looking good! Message configuration properly added.

The validation message handler now includes message configuration, which helps keep messages consistent.


117-118: Nice work! Permission and usage handlers updated consistently.

Both handlers now use the message configuration, which will help keep error messages consistent.


120-120: Perfect! Cooldown messages now use standard configuration.

The cooldown message handler has been updated to use the message configuration while keeping its existing functionality.


124-130: Well done! Commands updated consistently.

All commands now use the message configuration, which will make the messages more consistent throughout the plugin.

eternaleconomy-core/src/main/java/com/eternalcode/economy/command/handler/InvalidUsageHandlerImpl.java (3)

3-3: Nice addition of the message config!

The new field and import look good. They'll help keep message formatting consistent across the app.

Also applies to: 15-15


17-22: Constructor update looks clean!

The constructor nicely handles the new message config parameter. Good job initializing it in the field.


37-37: Good use of the prefix placeholder!

You've consistently added the {PREFIX} placeholder in both usage message spots. This matches the standardization effort across the project.

Just a quick check - make sure all your message templates in the config include this {PREFIX} placeholder.

Also applies to: 51-51

✅ Verification successful

🏁 Script executed:

#!/bin/bash
# Check if message templates include the {PREFIX} placeholder
rg -l "correctUsage|correctUsageHeader|correctUsageEntry" | xargs rg "{PREFIX}"

Length of output: 165


Below is a new shell script you can run that uses a fixed string match (via the –F flag) to search for the literal "{PREFIX}" in files containing the relevant message templates. This should avoid the regex parse error we saw before:

Please run this updated script to manually verify that all associated message templates include the "{PREFIX}" placeholder as intended.


🏁 Script executed:

#!/bin/bash
echo "Searching for literal '{PREFIX}' in message template files..."

# Find files with any of the key terms and then check them for the literal "{PREFIX}"
rg -l "correctUsage|correctUsageHeader|correctUsageEntry" | while read -r file; do
    echo "File: $file"
    rg -F "{PREFIX}" "$file"
done

Length of output: 1585


{PREFIX} Placeholder Verified!

The results confirm that the {PREFIX} placeholder is present in both the message templates within MessageConfig.java and in the handler in InvalidUsageHandlerImpl.java.

  • eternaleconomy-core/src/main/java/com/eternalcode/economy/config/implementation/messages/MessageConfig.java
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/command/handler/InvalidUsageHandlerImpl.java
eternaleconomy-core/src/main/java/com/eternalcode/economy/command/admin/AdminResetCommand.java (4)

6-6: Looks good - import added for new message config!

The new import aligns with the changes in message handling.


21-21: Nice addition of the message config field!

The private final field follows good Java practices.


25-27: Constructor updated correctly!

The MessageConfig parameter is properly added and initialized.

Also applies to: 30-30


40-40: Great use of the new prefix placeholder!

The prefix is consistently applied to both admin and player messages.

Let's make sure the prefix is used consistently across other commands:

Also applies to: 47-47

✅ Verification successful

🏁 Script executed:

#!/bin/bash
# Check if other commands use the same prefix pattern
rg -l "placeholder\(\"{PREFIX}\", messageConfig.messagesPrefix\)" | grep -v "AdminResetCommand"

Length of output: 246


Let's re-run the search using a fixed-string search to avoid regex issues. Try the following script:

This should help verify if the prefix placeholder is consistently applied elsewhere. Thanks!


🏁 Script executed:

#!/bin/bash
# Use fixed-string search to check for the prefix placeholder usage across commands,
# while excluding AdminResetCommand.
rg -F -l 'placeholder("{PREFIX}", messageConfig.messagesPrefix)' | grep -v "AdminResetCommand"

Length of output: 1210


Prefix Usage Confirmed Consistently Across the Codebase

The fixed-string search shows that the prefix placeholder is used not only in AdminResetCommand but also in multiple other commands and message handlers:

  • eternaleconomy-core/src/main/java/com/eternalcode/economy/command/admin/AdminSetCommand.java
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/command/admin/AdminBalanceCommand.java
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/command/admin/AdminAddCommand.java
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/command/admin/AdminRemoveCommand.java
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/command/handler/MissingPermissionHandlerImpl.java
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/command/cooldown/CommandCooldownMessage.java
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/command/handler/InvalidUsageHandlerImpl.java
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/command/message/InvalidBigDecimalMessage.java
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/command/player/MoneyTransferCommand.java
  • eternaleconomy-core/src/main/java/com/eternalcode/economy/command/player/MoneyBalanceCommand.java

This confirms the prefix placeholder is applied consistently. Great job!

eternaleconomy-core/src/main/java/com/eternalcode/economy/command/admin/AdminAddCommand.java (3)

6-6: Nice addition of the message config!

The new MessageConfig will help keep message formatting consistent across the application.

Also applies to: 25-25


30-32: Constructor update looks good!

The MessageConfig is properly injected and initialized.

Also applies to: 36-36


47-47: Good use of the prefix placeholder!

The PREFIX placeholder is consistently applied to both admin and player notifications.

However, let's make sure this matches other commands:

Also applies to: 55-55

✅ Verification successful

🏁 Script executed:

#!/bin/bash
# Check if other admin commands use the same PREFIX placeholder
ast-grep --pattern 'placeholder("{PREFIX}", messageConfig.messagesPrefix)'

Length of output: 76


I'm not seeing any output from the first command, so I'd like to re-run a broader search within the admin commands to verify if they consistently use the PREFIX placeholder. Let's run the following shell script:


🏁 Script executed:

#!/bin/bash
find . -type f -path "eternaleconomy-core/src/main/java/com/eternalcode/economy/command/admin/*.java" | xargs rg 'placeholder\("{PREFIX}",\s*messageConfig\.messagesPrefix\)' -n

Length of output: 334


Let's try verifying using a literal search mode to avoid regex issues. I'll run a new shell script to check for the use of our PREFIX placeholder in admin commands:


🏁 Script executed:

#!/bin/bash
find . -type f -path "eternaleconomy-core/src/main/java/com/eternalcode/economy/command/admin/*.java" | xargs rg -F 'placeholder("{PREFIX}", messageConfig.messagesPrefix)' -n

Length of output: 3726


Consistent use of PREFIX placeholder confirmed!

The search output shows that the PREFIX placeholder is used consistently across admin commands, including in both AdminAddCommand lines 47 and 55, as well as in similar patterns in other admin command files.

eternaleconomy-core/src/main/java/com/eternalcode/economy/EconomyReloadCommand.java (3)

4-4: LGTM! New import looks good.

The MessageConfig import matches the changes being made.


20-21: LGTM! Nice prefix update.

The message now uses the standardized {PREFIX} placeholder instead of a hardcoded value.


41-41: 🏁 Script executed:

#!/bin/bash
# Check if MessageConfig is marked as final
ast-grep --pattern 'class MessageConfig {
  $$$
}'

Length of output: 52

eternaleconomy-core/src/main/java/com/eternalcode/economy/command/admin/AdminSetCommand.java (4)

6-7: Looks good! 👍

The new imports are clean and well-organized.

Also applies to: 17-18


26-26: Perfect! ✨

The new MessageConfig field follows the same pattern as other fields.


28-38: Nice work! 🎯

The constructor changes are clean and well-structured.


44-57: 🏁 Script executed:

#!/bin/bash
# Look for notice templates that should contain {PREFIX}
rg -l "notice\.admin\.set|notice\.player\.set" 

Length of output: 141

eternaleconomy-core/src/main/java/com/eternalcode/economy/command/admin/AdminRemoveCommand.java (4)

6-6: Nice addition of the MessageConfig import!

The import aligns well with the new message prefix functionality.


25-25: Good job adding the MessageConfig field!

The private final field follows the same pattern as other dependencies in the class.


30-31: Constructor update looks good!

The MessageConfig parameter is properly added and initialized.

Also applies to: 36-36


47-47: Message prefix placeholders are consistently applied!

The prefix placeholder is added to all three notification messages, maintaining a uniform appearance.

However, let's make sure this prefix is used consistently across the entire codebase.

Also applies to: 60-60, 67-67

✅ Verification successful

🏁 Script executed:

#!/bin/bash
# Check if the message prefix is consistently used in all notification messages
rg -l "notice\(" | xargs rg "{PREFIX}"

Length of output: 124


I'll re-run the verification with a new shell script that uses fixed-string search (to avoid regex issues) and specifically checks for the prefix placeholder in notification messages.


🏁 Script executed:

#!/bin/bash
# Search for all instances of the fixed string '.placeholder("{PREFIX}",' in Java files
echo "Searching for message prefix placeholder usage in Java files:"
rg -Tjava -F '.placeholder("{PREFIX}",' --line-number

# Specifically check the AdminRemoveCommand.java file for occurrences of '{PREFIX}'
echo "Checking AdminRemoveCommand.java for {PREFIX} occurrences:"
grep -n '{PREFIX}' eternaleconomy-core/src/main/java/com/eternalcode/economy/command/admin/AdminRemoveCommand.java

Length of output: 646


Prefix placeholder usage is consistent.

Our search confirmed that the prefix placeholder is used on lines 47, 60, and 67 in AdminRemoveCommand.java, aligning with the intended consistency.

@VerniQ VerniQ requested review from Rollczi and CitralFlo February 8, 2025 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Template/prefix for notifcations.
4 participants