refactor: Brigadier commands - #18
Conversation
d71f450 to
01386b8
Compare
amyavi
left a comment
amyavi
left a comment
There was a problem hiding this comment.
Not a maintainer, but here's my review
| import java.util.List; | ||
|
|
||
| public interface BrigadierCommand { | ||
| String getLabel(); |
| private final PlayerControl controlModule; | ||
|
|
||
| public CommandIcu(final PlayerControl controlModule) { | ||
| public CommandIcu(PlayerControl controlModule) { |
There was a problem hiding this comment.
Why was this final here dropped?
| TranslationStore.StringBased<MessageFormat> store = | ||
| TranslationStore.messageFormat( | ||
| Key.key("icontrolu", "translations") | ||
| ); | ||
| ResourceBundle english = ResourceBundle.getBundle("lang.messages", Locale.US); | ||
| store.registerAll(Locale.US, english, true); | ||
| GlobalTranslator.translator().addSource(store); | ||
|
|
There was a problem hiding this comment.
Given translations are not being used, this isn't required.
| = Commands.literal(command.getLabel()); | ||
| command.build(builder); | ||
| registrar.register( | ||
| builder.build(),command.getDescription(),command.getAliases() |
There was a problem hiding this comment.
Spaces after the commas, please
| description: Control another player's movements, inventory and chat | ||
| permission: icu.command | ||
| api-version: '26.2' | ||
| version: master No newline at end of file |
There was a problem hiding this comment.
Keep the newline at the end of file there
| .requires(src -> | ||
| src.getSender().hasPermission("icu.command") | ||
| && src.getSender() instanceof Player | ||
| ) |
There was a problem hiding this comment.
This should probably be restricted, to prevent players from accidentally controlling others when clicking something in chat: https://docs.papermc.io/paper/dev/command-api/basics/requirements/#restricted-commands
| <groupId>io.papermc.paper</groupId> | ||
| <artifactId>paper-api</artifactId> | ||
| <version>1.18.2-R0.1-SNAPSHOT</version> | ||
| <version>26.2.build.112-stable</version> |
There was a problem hiding this comment.
Please consider using [26.2.build,) instead so we always target the latest Paper API.
|
|
||
| import java.util.List; | ||
|
|
||
| public interface BrigadierCommand { |
There was a problem hiding this comment.
There is no use for such an interface if we only implement a single command. Additionally, it will be a pain to maintain updates to this interface across projects, if that is the plan, without some shared "common" plugin.
Therefore, I think it would be better to inline the builder, applying such abstractions to Brigadier commands is, in my opinion, a misbegotten attempt to stuff it into a Bukkit-style box.
Please take a look at https://github.com/kaboomserver/commandspy/blob/dd6d940d811d354d7e3a8693b9fef3aeb7b836c2/src/main/java/pw/kaboom/commandspy/Main.java#L59 for an example of such usage.
| controlCommand(controller, label, args); | ||
| return true; | ||
| } | ||
| public void build(LiteralArgumentBuilder<CommandSourceStack> builder) { |
There was a problem hiding this comment.
Please make formatting consistent with https://github.com/kaboomserver/commandspy/blob/dd6d940d811d354d7e3a8693b9fef3aeb7b836c2/src/main/java/pw/kaboom/commandspy/Main.java#L59
01386b8 to
197f649
Compare
| private static Player getSender(CommandContext<CommandSourceStack> ctx) | ||
| throws CommandSyntaxException { | ||
| if (ctx.getSource().getSender() instanceof Player player) { | ||
| return player; | ||
| } | ||
| return true; | ||
| throw EX_NOT_PLAYER.create(); | ||
| } |
There was a problem hiding this comment.
The source will always be an instanceof Player, due to the earlier requires check. However, I recognize the utility of this function, so I think that it should be replaced with:
| private static Player getSender(CommandContext<CommandSourceStack> ctx) | |
| throws CommandSyntaxException { | |
| if (ctx.getSource().getSender() instanceof Player player) { | |
| return player; | |
| } | |
| return true; | |
| throw EX_NOT_PLAYER.create(); | |
| } | |
| private static Player getPlayer(CommandContext<CommandSourceStack> ctx) | |
| return (Player) ctx.getSource().getSender(); | |
| } |
197f649 to
3c3946c
Compare
3c3946c to
ded841b
Compare
|
|
||
| if (args[0].equalsIgnoreCase("stop")) { | ||
| stopCommand(controller); | ||
| private static String getPlayerName(Object player) { |
Co-authored-by: opt <173472493+OptimisticDeving@users.noreply.github.com>
3 participants
No description provided.