Bump down version and add readme and banner by CitralFlo · Pull Request #54 · EternalCodeTeam/multification · GitHub
Skip to content

Bump down version and add readme and banner#54

Closed
CitralFlo wants to merge 32 commits into
masterfrom
bump-down-version
Closed

Bump down version and add readme and banner#54
CitralFlo wants to merge 32 commits into
masterfrom
bump-down-version

Conversation

@CitralFlo

Copy link
Copy Markdown
Member

No description provided.

@coderabbitai

coderabbitai Bot commented Oct 17, 2024

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 7

🧹 Outside diff range and nitpick comments (8)
buildSrc/src/main/kotlin/multification-repositories.gradle.kts (1)

13-13: Looks good! Maybe add a space?

The new repository is a nice addition. For consistency with other lines, consider adding a space after the comment slash.

-    maven("https://s01.oss.sonatype.org/content/repositories/snapshots/") // adventure snapshots
+    maven("https://s01.oss.sonatype.org/content/repositories/snapshots/") //  adventure snapshots
multification-core/src/com/eternalcode/multification/notice/resolver/bossbar/BossBarContent.java (1)

11-11: Nice job on the record!

The BossBarContent record looks good. It covers all the needed info for a boss bar.

Quick tip: You could add a short comment explaining what this record does.

examples/bukkit/src/main/java/com/eternalcode/example/bukkit/command/timer/TimerCommand.java (1)

18-21: The execute method works, but could be more flexible.

Maybe consider allowing different timer durations?

examples/bukkit/src/main/java/com/eternalcode/example/bukkit/command/timer/TimerManager.java (1)

24-36: Let's make the timer even better!

The tickTimer method works, but we can improve it:

  1. Maybe add a check for negative seconds?
  2. For very long timers, we might want to use a different approach.

Want help making these changes?

examples/bukkit/src/main/java/com/eternalcode/example/bukkit/ExamplePlugin.java (1)

41-42: Command setup looks good, but could be even better!

Nice job adding the TimerCommand! It's great that you're using the plugin's resources for the TimerManager.

A small suggestion: Consider moving the TimerManager creation to a separate method. It'll make the code even easier to read!

README.md (2)

1-13: Add alt text to the banner image

Hey there! The banner looks great, but let's make it even better by adding some alt text. This helps everyone understand what's in the image, even if they can't see it.

Try updating line 3 like this:

-![](/assets/readme-banner.png)
+![Multification Banner](/assets/readme-banner.png)
🧰 Tools
🪛 Markdownlint

3-3: null
Images should have alternate text (alt text)

(MD045, no-alt-text)


99-101: Small tweaks for the final section

Great info here! Let's make it even better with a few small changes:

  1. In line 99, try: "To add messages to the configuration, create a variable in the config with class Notice or BukkitNotice."
  2. For line 101, let's remove the period at the end of the heading.

These little fixes will make your README shine even more!

Here's how you could update it:

-Setting up configuration is easy both in cdn and okaeri configs. To add messages to the configuration, create variable in config with class `Notice` or `BukktiNotice`. You can also use builder. After plugin deploy you can find messages in configuration file.
+Setting up configuration is easy both in cdn and okaeri configs. To add messages to the configuration, create a variable in the config with class `Notice` or `BukkitNotice`. You can also use a builder. After plugin deployment, you can find messages in the configuration file.

-### To see more examples open the [example plugin module](https://github.com/EternalCodeTeam/multification/tree/master/examples/bukkit).
+### To see more examples open the [example plugin module](https://github.com/EternalCodeTeam/multification/tree/master/examples/bukkit)
🧰 Tools
🪛 LanguageTool

[uncategorized] ~99-~99: You might be missing the article “a” here.
Context: ...d messages to the configuration, create variable in config with class Notice or `Bukkt...

(AI_EN_LECTOR_MISSING_DETERMINER_A)


[uncategorized] ~99-~99: You might be missing the article “the” here.
Context: ...uration, create variable in config with class Notice or BukktiNotice. You can als...

(AI_EN_LECTOR_MISSING_DETERMINER_THE)


[uncategorized] ~101-~101: Possible missing comma found.
Context: ...in configuration file. ### To see more examples open the [example plugin module](https:...

(AI_HYDRA_LEO_MISSING_COMMA)

🪛 Markdownlint

101-101: Punctuation: '.'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)

multification-okaeri/test/com/eternalcode/multification/okaeri/NoticeSerializerTest.java (1)

583-583: Quick note: Be cautious with string replacements

In the removeBlankNewLines method, removing all quotation marks might affect the expected YAML strings.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between f2e7ec4 and dbfe28f.

⛔ Files ignored due to path filters (1)
  • assets/readme-banner.png is excluded by !**/*.png
📒 Files selected for processing (19)
  • README.md (1 hunks)
  • buildSrc/src/main/kotlin/multification-repositories.gradle.kts (1 hunks)
  • examples/bukkit/src/main/java/com/eternalcode/example/bukkit/ExamplePlugin.java (2 hunks)
  • examples/bukkit/src/main/java/com/eternalcode/example/bukkit/command/timer/TimerCommand.java (1 hunks)
  • examples/bukkit/src/main/java/com/eternalcode/example/bukkit/command/timer/TimerManager.java (1 hunks)
  • examples/bukkit/src/main/java/com/eternalcode/example/bukkit/config/MessagesConfig.java (3 hunks)
  • multification-cdn/src/com/eternalcode/multification/cdn/MultificationNoticeCdnComposer.java (9 hunks)
  • multification-cdn/test/com/eternalcode/multification/cdn/NoticeComposerTest.java (14 hunks)
  • multification-core/src/com/eternalcode/multification/notice/Notice.java (4 hunks)
  • multification-core/src/com/eternalcode/multification/notice/NoticeBroadcastImpl.java (1 hunks)
  • multification-core/src/com/eternalcode/multification/notice/NoticeKey.java (2 hunks)
  • multification-core/src/com/eternalcode/multification/notice/resolver/NoticeResolverDefaults.java (2 hunks)
  • multification-core/src/com/eternalcode/multification/notice/resolver/NoticeSerdesResult.java (2 hunks)
  • multification-core/src/com/eternalcode/multification/notice/resolver/bossbar/BossBarContent.java (1 hunks)
  • multification-core/src/com/eternalcode/multification/notice/resolver/bossbar/BossBarResolver.java (1 hunks)
  • multification-core/src/com/eternalcode/multification/notice/resolver/bossbar/BossBarService.java (1 hunks)
  • multification-core/test/com/eternalcode/multification/MultificationTest.java (2 hunks)
  • multification-okaeri/src/com/eternalcode/multification/okaeri/MultificationNoticeSerializer.java (3 hunks)
  • multification-okaeri/test/com/eternalcode/multification/okaeri/NoticeSerializerTest.java (16 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md

[uncategorized] ~99-~99: You might be missing the article “a” here.
Context: ...d messages to the configuration, create variable in config with class Notice or `Bukkt...

(AI_EN_LECTOR_MISSING_DETERMINER_A)


[uncategorized] ~99-~99: You might be missing the article “the” here.
Context: ...uration, create variable in config with class Notice or BukktiNotice. You can als...

(AI_EN_LECTOR_MISSING_DETERMINER_THE)


[uncategorized] ~101-~101: Possible missing comma found.
Context: ...in configuration file. ### To see more examples open the [example plugin module](https:...

(AI_HYDRA_LEO_MISSING_COMMA)

🪛 Markdownlint
README.md

101-101: Punctuation: '.'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)


3-3: null
Images should have alternate text (alt text)

(MD045, no-alt-text)

🔇 Additional comments (58)
multification-core/src/com/eternalcode/multification/notice/resolver/bossbar/BossBarContent.java (2)

1-9: Looks good!

The package and imports are set up correctly.


13-16: Good work on the method!

The contents() method does exactly what it needs to do. It's simple and clear.

examples/bukkit/src/main/java/com/eternalcode/example/bukkit/command/timer/TimerCommand.java (4)

1-7: Looks good!

The package and imports are set up nicely.


8-9: Good job on the annotations!

The command name and permission are clearly defined.


10-12: Nice and clean class setup!

The class and field are well-defined.


14-16: Good constructor!

It's set up correctly to initialize the timer manager.

multification-core/src/com/eternalcode/multification/notice/resolver/NoticeSerdesResult.java (2)

4-4: Looks good!

The new import is needed for the Section record. Nice job adding it.


36-41: Can you explain the Section record?

The new Section record looks okay, but I'm not sure why it's needed. Could you add a comment explaining what it's for? Also, why does anyElements() return an empty list?

examples/bukkit/src/main/java/com/eternalcode/example/bukkit/command/timer/TimerManager.java (3)

1-8: Good job on the setup!

The class name and imports look good. They match what the class does.


10-18: Nice work on the constructor!

The fields and constructor look good. They're set up correctly.


20-22: Good start to the timer!

The startTimer method looks simple and does its job well.

multification-core/src/com/eternalcode/multification/notice/NoticeKey.java (2)

5-5: New import looks good!

The import for BossBarContent fits right in with the other imports.


21-21: Nice addition of BOSS_BAR constant!

The new BOSS_BAR constant matches the style of the other constants. Good job keeping things consistent!

multification-core/src/com/eternalcode/multification/notice/resolver/NoticeResolverDefaults.java (2)

4-5: New imports look good!

These imports will help with the new boss bar feature. Nice addition!


31-31: Great job adding the boss bar resolver!

The new boss bar resolver fits in nicely with the other resolvers. It's a good addition to the registry.

examples/bukkit/src/main/java/com/eternalcode/example/bukkit/config/MessagesConfig.java (3)

6-6: New imports look good!

The added imports for BossBar and Duration are needed for the new timer feature. Nice job!

Also applies to: 10-10


25-25: Cool gradient effect!

The new subtitle looks great with the gradient colors. It's a nice touch that makes the message pop!


35-38: Awesome boss bar timer!

The new bossbarTimer looks great! It adds a cool feature for showing timers to players. Good job on using a placeholder for the time too!

examples/bukkit/src/main/java/com/eternalcode/example/bukkit/ExamplePlugin.java (2)

5-6: New imports look good!

The TimerCommand and TimerManager imports are in the right place. They'll help with the new timer feature.


5-6: Great job adding the timer feature!

You've smoothly integrated the new timer functionality into the plugin. The imports and command setup look good. This update will give users more options, which is awesome!

Also applies to: 41-42

README.md (3)

15-30: Great job on the introduction!

The intro looks spot-on! It's clear and covers all the key points about what Multification can do. Nice work!


32-39: Setup instructions look good

The setup part is clear and easy to follow. You've made it simple for people to add Multification to their projects. Well done!


41-97: Code examples are helpful

These examples are great! They show exactly how to set up and use Multification in a project. This will be super helpful for users.

multification-core/test/com/eternalcode/multification/MultificationTest.java (1)

104-104: Nice touch with the test name!

Adding a display name makes the test purpose clearer. Good job!

multification-okaeri/src/com/eternalcode/multification/okaeri/MultificationNoticeSerializer.java (1)

61-63: Handling 'Section' results looks good!

The addition for serializing NoticeSerdesResult.Section is correct.

multification-core/src/com/eternalcode/multification/notice/Notice.java (2)

107-130: Great addition of bossBar methods!

The new static methods for boss bar notifications are a valuable enhancement.


218-233: Builder methods for bossBar look good!

The builder now offers flexible options for creating boss bar notices.

multification-core/src/com/eternalcode/multification/notice/NoticeBroadcastImpl.java (1)

325-328: Nice addition of the getThis() method!

Including this method helps with method chaining and keeps the code clean.

multification-cdn/test/com/eternalcode/multification/cdn/NoticeComposerTest.java (9)

331-333: Great addition of boss bar tests

Adding the ConfigBossbarWOBuilder class improves test coverage for boss bar serialization without using the builder.


335-359: Test methods are well-structured

The test serializeBossBarSectionWithAllPropertiesWOBuilder effectively verifies serialization with all properties.


361-387: Good job testing without progress

Your tests for boss bar serialization without progress ensure correct handling when progress isn't specified.


389-417: Effective testing without overlay

The ConfigBossbarWithoutOverlayWOBuilder class and its test confirm proper behavior when overlay is omitted.


419-447: Comprehensive tests for minimal configurations

Including tests without both progress and overlay enhances confidence in default behavior handling.


449-478: Nice use of builder pattern

The ConfigBossBarWithAllProperties class demonstrates effective use of the builder for boss bar configuration.


481-509: Thorough testing without progress

Your test ensures that boss bar serialization works correctly when progress isn't provided.


511-542: Solid verification without overlay

Testing boss bar serialization without an overlay confirms the robustness of default settings.


544-574: Ensuring functionality with minimal settings

The test for boss bar without progress and overlay confirms proper handling of minimal configurations.

multification-okaeri/test/com/eternalcode/multification/okaeri/NoticeSerializerTest.java (16)

322-324: Great addition of the ConfigBossbarWOBuilder class!

The boss bar notice configuration is clear and easy to understand.


326-339: Test method looks solid

The serializeBossBarWOBuilderWithAllProperties test correctly verifies serialization with all properties.


353-354: Nice work on ConfigBossbarWithoutProgressWOBuilder

This class effectively tests serialization without the progress property.


356-369: Test covers the scenario well

The serializeBossBarWOBuilderWithoutProgressProperty test checks serialization without the progress property as expected.


381-382: ConfigBossbarWithoutOverlayWOBuilder looks good

You're properly testing the absence of the overlay property.


384-396: Test method is well-implemented

The serializeBossBarWOBuilderWithoutOverlayProperty test verifies serialization without the overlay property effectively.


412-413: ConfigBossbarWithoutProgressAndOverlayWOBuilder is well-defined

This class tests serialization without both progress and overlay properties nicely.


415-426: Test accurately checks serialization

The serializeBossBarWOBuilderWithoutProgressAndOverlayProperties test validates the serialization without progress and overlay properties.


441-445: Good use of the builder pattern in ConfigBossBar

This enhances readability and makes the code easy to maintain.


447-470: Test effectively verifies boss bar serialization

The serializeBossBarSectionWithAllProperties test checks all properties are serialized correctly.


472-476: ConfigBossBarWithoutProgress is well-crafted

Testing without the progress property is handled well here.


478-500: Test properly verifies absence of progress property

The serializeBossBarSectionWithoutProgressProperty test functions as expected.


502-506: ConfigBossBarWithoutOverlay looks good

This configuration tests serialization without the overlay property effectively.


508-533: Test correctly checks serialization without overlay

The serializeBossBarSectionWithoutOverlayProperty test validates the intended behavior.


535-539: ConfigBossBarWithoutProgressAndOverlay is well-designed

You're covering the scenario without both progress and overlay properties nicely.


541-565: Test effectively handles serialization without progress and overlay

The serializeBossBarSectionWithoutProgressAndOverlayProperties test confirms the expected serialization.

multification-cdn/src/com/eternalcode/multification/cdn/MultificationNoticeCdnComposer.java (5)

14-14: Imports look good!

The necessary classes are properly imported.

Also applies to: 16-16, 27-27


46-46: Method signature updated appropriately.

Adding String key enhances the method's functionality.


110-114: Great addition for handling sections.

This improves the serialization logic.


139-146: New method toNamedSection is a helpful addition.

It enhances how sections are serialized.


215-218: Double-check result.project() usage.

Please ensure it returns the intended result in this context.


public class BossBarService {

private final ScheduledExecutorService scheduler = Executors.newScheduledThreadPool(8);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider using fewer threads.

An 8-thread pool might be more than necessary. A smaller pool could work just as well.

}

Duration remaining = Duration.between(Instant.now(), expiration);
float progress = 1 - (float) remaining.getSeconds() / duration.getSeconds();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid division by zero.

If duration.getSeconds() is zero, this will cause an error. Please add a check to prevent this.

Comment on lines +68 to +70
BossBar.Color color = BossBar.Color.valueOf(sectionResult.elements().getOrDefault("color", DEFAULT_COLOR));
Duration duration = DurationParser.TIME_UNITS.parse(sectionResult.elements().getOrDefault("duration", DEFAULT_DURATION));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle invalid color input

If the 'color' value is invalid, it might cause an exception. Let's add a check to use the default color when needed.

Comment on lines +75 to +80
if (sectionResult.elements().get("overlay") != null) {
overlay = Optional.of(BossBar.Overlay.valueOf(sectionResult.elements().get("overlay")));
}
else {
overlay = Optional.empty();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle invalid overlay input

An invalid 'overlay' value could lead to exceptions. It's a good idea to default to a safe value if parsing fails.

Comment on lines +93 to +95
double progress = Double.parseDouble(rawProgress);
return OptionalDouble.of(progress);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure 'progress' is within valid range

Let's validate that 'progress' is between 0.0 and 1.0 to prevent unexpected behavior.

Comment on lines +112 to +120
if (value instanceof Map<?, ?> mapValue) {
NoticeDeserializeResult<?> noticeResult = this.noticeRegistry.deserialize(key, new NoticeSerdesResult.Section((Map<String, String>) mapValue))
.orElseThrow(() -> new UnsupportedOperationException(
"Unsupported notice key: " + key + " with values: " + mapValue));

this.withPart(builder, noticeResult);
continue;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure safe casting in deserialization.

Casting mapValue to (Map<String, String>) may cause issues if the map doesn't contain String keys and values. Please check the map's contents before casting to prevent potential errors.

Comment on lines +240 to +254

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider simplifying similar code in appendEntry and appendSection.

Combining shared logic might make the code cleaner.

Also applies to: 255-286

@CitralFlo CitralFlo closed this Oct 17, 2024
@CitralFlo CitralFlo deleted the bump-down-version branch October 17, 2024 17:42
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.

3 participants