-
-
Notifications
You must be signed in to change notification settings - Fork 21
GH-1129 Replace @Context annotation with @Sender where possible #1129
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
Conversation
/gemini review |
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.
Pull Request Overview
This PR replaces the @Context
annotation with @Sender
annotation in command method parameters where the parameter represents the command sender. This is a refactoring effort to use more semantically appropriate annotations for command sender parameters in the LiteCommands framework.
- Systematically replaces
@Context Player
and@Context CommandSender
with@Sender
where the parameter represents the command executor - Updates import statements to include
@Sender
and remove unused@Context
imports where applicable - Maintains existing
@Context
usage for other parameters likeViewer
,User
, andInvocation
which represent different contextual data
Reviewed Changes
Copilot reviewed 72 out of 72 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
Command classes (65+ files) | Replace @Context with @Sender for Player/CommandSender parameters representing command executors |
Import statements | Add @Sender imports and clean up unused @Context imports where appropriate |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
eternalcore-core/src/main/java/com/eternalcode/core/feature/sudo/SudoCommand.java
Outdated
Show resolved
Hide resolved
...e/src/main/java/com/eternalcode/core/feature/teleport/command/TeleportToPositionCommand.java
Outdated
Show resolved
Hide resolved
...e/src/main/java/com/eternalcode/core/feature/teleport/command/TeleportToPositionCommand.java
Outdated
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.
Code Review
This pull request correctly refactors the codebase to use the @Sender
annotation instead of @Context
for injecting the command sender. The changes are consistent across most of the project. However, I've identified a couple of issues. One is a functional regression in TeleportToPositionCommand
where a refactoring removed functionality. Another is a minor code clarity issue in SudoCommand
. Please see the detailed comments for suggestions.
...e/src/main/java/com/eternalcode/core/feature/teleport/command/TeleportToPositionCommand.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/sudo/SudoCommand.java
Outdated
Show resolved
Hide resolved
…do/SudoCommand.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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.
Code Review
This pull request correctly replaces the @Context
annotation with the more specific @Sender
annotation across multiple command classes, which improves code clarity and adheres to the litecommands
library's best practices. The changes are consistent and well-executed. However, I've identified a functional regression in TeleportToPositionCommand
where a change in method call removes the feedback message to the player after teleportation, and an unused parameter in SudoCommand
. I've provided suggestions to fix these issues.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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 I’m being honest, I really don’t like this change. In my opinion, |
I did not implement everywhere in Viewer type as it would cause confusion, for example in method execute(@sender Viewer viewer, @sender CommandSender sender) it just looks bad I can search for use cases where I can indeed use this annotation for Viewer |
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 these are all the cases, it looks ok :)
No description provided.