-
-
Notifications
You must be signed in to change notification settings - Fork 17
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
GH-830 Teleport to default home feature #858
Conversation
Took 15 minutes
Took 2 minutes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ficzer jest już w eternacore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A dobra, już czaję koncept ale boolean w config jest trochę useless. Jak coś jest default to powinno tepać tam i tyle po wpisaniu /home
eternalcore-core/src/main/java/com/eternalcode/core/feature/home/command/HomeCommand.java
Show resolved
Hide resolved
Took 10 minutes
WalkthroughThe changes in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- eternalcore-core/src/main/java/com/eternalcode/core/feature/home/command/HomeCommand.java (3 hunks)
🧰 Additional context used
🔇 Additional comments (2)
eternalcore-core/src/main/java/com/eternalcode/core/feature/home/command/HomeCommand.java (2)
55-56
: Ensure deterministic retrieval offirstHome
.Using
playerHomes.iterator().next()
may not consistently return the same home ifplayerHomes
is backed by an unordered collection, such as aHashSet
. This could lead to unpredictable behavior when teleporting the player to their first home.Please confirm that
playerHomes
is an ordered collection (e.g., aList
) to ensure thatfirstHome
is consistent across calls.
64-71
: Previous review comment applies: Simplify code by usingfirstHome
instead of configuration.The suggestion to remove the dependency on
homesConfig.defaultHomeName
and usefirstHome
is still valid. This can simplify the code and reduce reliance on external configuration.Note: CitralFlo previously commented: "please remove Boolean from config and make use of existing firstHome value on line 81 if possible simplifying the code."
eternalcore-core/src/main/java/com/eternalcode/core/feature/home/command/HomeCommand.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have tested, everything in my opinion is good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Took 3 minutes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- eternalcore-core/src/main/java/com/eternalcode/core/feature/home/command/HomeCommand.java (3 hunks)
🧰 Additional context used
🔇 Additional comments (5)
eternalcore-core/src/main/java/com/eternalcode/core/feature/home/command/HomeCommand.java (5)
4-4
: LGTM: New imports are appropriate for the changes.The added imports for
PluginConfiguration
andjava.util.Optional
are necessary and correctly support the new functionality in theHomeCommand
class.Also applies to: 16-17
24-24
: LGTM: New field for homes configuration.The addition of the
homesConfig
field is appropriate for storing home-related configuration. It's correctly declared as private and final, adhering to good encapsulation practices.
31-31
: LGTM: Constructor updated to include homes configuration.The constructor has been correctly updated to include the new
homesConfig
parameter, which is then properly assigned to the class field. This change adheres to dependency injection principles and ensures theHomeCommand
class has access to the necessary configuration.Also applies to: 36-36
55-70
: LGTM: Improved handling of multiple homes and default home concept.The changes to the
execute
method enhance the functionality by introducing a default home concept and improving the handling of multiple homes. The logic is more flexible and user-friendly, allowing for default home teleportation when applicable.
66-70
: Address previous review comments.Regarding past review comments:
The suggestion to provide a fallback to
firstHome
when the default home is not found is still valid. This aligns with the refactoring suggestion in the previous comment.The comment about removing
Boolean
from config and usingfirstHome
doesn't seem directly applicable to the current code. There's no visibleBoolean
config orfirstHome
usage in the provided snippet. If this was implemented elsewhere, please ensure it's consistent with the current implementation.Please review these past suggestions and ensure they're addressed or clarify if they're no longer applicable.
Summary by CodeRabbit
New Features
Bug Fixes