-
-
Notifications
You must be signed in to change notification settings - Fork 66
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
Type-safe Kotlin DSL #544
Comments
Hmm, interesting. |
@DerEchtePilz Thank you for your interest. We will need to make a breaking change to "optional". Currently, specifying For example: commandTree("sendmessageto") {
playerArgument("player", /* Actually: optional = false */) { getPlayer ->
greedyStringArgument("msg", optional = true) { getMessage ->
anyExecutor { _, args ->
val player = getPlayer(args) // Expected: Player (not-null)
val message = getMessage(args) // Expected: String? (nullable)
player.sendMessage(message)
}
}
}
} However, depending on the value of the argument and changing the type is difficult and requires changing the method to call. commandTree("sendmessageto") {
playerArgument("player") { getPlayer ->
greedyStringOptionalArgument("msg") { getMessage ->
anyExecutor { _, args ->
val player = getPlayer(args) // Returns Player (not-null)
val message = getMessage(args) // Returns String? (nullable)
player.sendMessage(message)
}
}
}
} I would like to discuss this 🙏 |
@sya-ri On a different note, iirc @willkroboth currently is working on updating the way commands are registered and has also made changes to optional argument handling. During that, we also kinda decided that optional arguments don't really make sense in a
This will also never change. Optional arguments just have the risk of not being present which is why we return |
I hate that MultiLiteralArgument mess... A bit of history about that But yeah, those deprecations are already in the process of being removed, we just can't do it until 10.0.0 |
I see. Changes to this issue will also be merged in 10.0.0. Right? |
That solely depends on the nature of your PR and what it changes. |
OK. I will create a PR as soon as possible 😉 |
I wonder if this could be added for Java as well. If I add this method to public <T, N extends AbstractArgument<T, ?, Argument, CommandSender>> Impl then(final N tree, BiConsumer<Function<CommandArguments, T>, N> consumer) {
this.arguments.add(tree);
consumer.accept(args -> args.getUnchecked(tree.getNodeName()), tree);
return instance();
} Then this command syntax becomes possible: new CommandTree("sendMessageTo")
.then(new PlayerArgument("player"), (getPlayer, playerArgument) -> playerArgument
.then(new GreedyStringArgument("msg"), (getMessage, messageArgument) -> messageArgument
.executes((sender, args) -> {
Player player = getPlayer.apply(args);
String message = getMessage.apply(args);
player.sendMessage(message);
})
)
)
.register(); Java lambdas aren't as clean as Kotlin's, but it still hides the unsafe casting (calling With public <T> Impl withArgument(AbstractArgument<T, ?, Argument, CommandSender> argument, BiConsumer<Function<CommandArguments, T>, Impl> consumer) {
this.arguments.add((Argument) argument);
consumer.accept(args -> args.getUnchecked(argument.getNodeName()), instance());
return instance();
}
public <T> Impl withOptionalArgument(AbstractArgument<T, ?, Argument, CommandSender> argument, BiConsumer<OptionalArgumentProvider<T>, Impl> consumer) {
argument.setOptional(true);
this.arguments.add((Argument) argument);
consumer.accept(new OptionalArgumentProvider<>(argument), instance());
return instance();
} I can make a command like this: new CommandAPICommand("sayhi")
.withArgument(new PlayerArgument("target"), (getTarget, command1) -> command1
.withOptionalArgument(new GreedyStringArgument("message"), (getMessage, command2) -> command2 // This is a little annoying, where the lambda parameters must have different names --- Although command1 and command2 do refer to the same object (`new CommandAPICommand("sayhi")`), so maybe command2 can be removed
.executes((sender, args) -> {
// target is not only type-safe, but also guaranteed to be not null
Player target = getTarget.apply(args);
target.sendMessage("Hello!");
Optional<String> message = getMessage.getOptional(args);
if(message.isPresent()) {
target.sendMessage(message.get());
} else {
target.sendMessage("How are you doing?");
}
})))
.register(); For the optional argument here, I created an public class OptionalArgumentProvider<T> {
private final AbstractArgument<T, ?, ?, ?> argument;
public OptionalArgumentProvider(AbstractArgument<T, ?, ?, ?> argument) {
this.argument = argument;
}
@Nullable
public T get(CommandArguments args) {
return args.getUnchecked(this.argument.getNodeName());
}
@NotNull
public T getOrDefault(CommandArguments args, T defaultValue) {
return args.getOrDefaultUnchecked(this.argument.getNodeName(), defaultValue);
}
@NotNull
public Optional<T> getOptional(CommandArguments args) {
return args.getOptionalUnchecked(this.argument.getNodeName());
}
} Just a way of giving more options than a basic |
Add IntegerRangeArgumentType to let Brigadier parse input to IntegerRange. Currently, the exceptions thrown are just their raw strings. I couldn't seem to get the translations keys `argument.range.empty` and `argument.range.swapped` to resolve due to MockBukkit/MockBukkit#1040. Implement Parser builder to make defining object parsers easier. Inspired by Brigadier command parse trees and #544. Add IntegerRangeArgumentTests
Add IntegerRangeArgumentType to let Brigadier parse input to IntegerRange. Currently, the exceptions thrown are just their raw strings. I couldn't seem to get the translations keys `argument.range.empty` and `argument.range.swapped` to resolve due to MockBukkit/MockBukkit#1040. Implement Parser builder to make defining object parsers easier. Inspired by Brigadier command parse trees and #544. Add IntegerRangeArgumentTests
Add IntegerRangeArgumentType to let Brigadier parse input to IntegerRange. Currently, the exceptions thrown are just their raw strings. I couldn't seem to get the translations keys `argument.range.empty` and `argument.range.swapped` to resolve due to MockBukkit/MockBukkit#1040. Implement Parser builder to make defining object parsers easier. Inspired by Brigadier command parse trees and #544. Add IntegerRangeArgumentTests
This is a bit off-topic, but I'm not quite sure about that, and it seems to make sense to use Optional Argument in 'thenNested()' after implementing #529? |
Continuing the off topic, but it's fine :P
So, the thing about optional arguments in CommandTrees is explained more here: 193f237#r126326791. If we allow optional arguments anywhere in CommandTrees, it is possible to make ambiguous cases. Currently the CommandAPI doesn't do anything about this, so the command that actually ends up being registered depends on the internal logic of the CommandAPI, which leads to weird results like the final executor depending on the order of later branches. Some examples: // Creates:
// - /command -> "You ran the base command"
// Branches a and b are ignored
new CommandTree("command")
.executes(info -> info.sender().sendMessage("You ran the base command"))
.then(new LiteralArgument("a").setOptional(true))
.then(new LiteralArgument("b").setOptional(true))
.register();
// Creates:
// - /command -> "You ran branch b"
// - /command a -> "You ran branch a"
// - /command b -> "You ran branch b"
// Branch b overwrites base executor
new CommandTree("command")
.executes(info -> info.sender().sendMessage("You ran the base command"))
.then(new LiteralArgument("a").setOptional(true).executes(info -> info.sender().sendMessage("You ran branch a")))
.then(new LiteralArgument("b").setOptional(true).executes(info -> info.sender().sendMessage("You ran branch b")))
.register();
// Creates:
// - /command -> "You ran branch a"
// - /command a -> "You ran branch a"
// - /command b -> "You ran branch b"
// Branch a overwrites base executor
new CommandTree("command")
.executes(info -> info.sender().sendMessage("You ran the base command"))
.then(new LiteralArgument("b").setOptional(true).executes(info -> info.sender().sendMessage("You ran branch b")))
.then(new LiteralArgument("a").setOptional(true).executes(info -> info.sender().sendMessage("You ran branch a")))
.register(); Optional arguments are ambiguous in CommandTrees when a node with an executor has any optional branches, or when a node has at least 2 optional branches. Along the way of thinking about this, we did have a setup where CommandTrees had a But talking about this in Discord we thought that this was probably better handled directly by the user. I think private void handleCommand(CommandSender sender, CommandArguments args) {
String arg1 = args.getUnchecked("required1");
String arg2 = args.getUnchecked("required2");
assert arg1 != null && arg2 != null;
String arg3 = args.getUnchecked("optional");
if (arg3 != null) {
String arg4 = args.getUnchecked("required3");
assert arg4 != null
}
}
public void register() {
// CommandAPICommand
new CommandAPICommand("command")
.withArguments(
new StringArgument("required1"),
new StringArgument("required2")
)
.withOptionalArguments(
new StringArgument("optional").combineWith(
new StringArgument("required3")
)
)
.executes(this::handleCommand)
.register()
// Current CommandTree
new CommandTree("command")
.then(new StringArgument("required1")
.then(new StringArgument("required2").executes(this::handleCommand)
.then(new StringArgument("optional")
.then(new StringArgument("required3").executes(this::handleCommand))
)
)
)
.register()
// CommandTree with `thenNested`
new CommandTree("command")
.thenNested(
new StringArgument("required1"),
new StringArgument("required2").executes(this::handleCommand),
new StringArgument("optional"),
new StringArgument("required3").executes(this::handleCommand)
)
.register();
} Funnily |
yeah, kotlin dsl ver even more better:
|
Description
The current Kotlin DSL requires casts, so it is not type-safe. We can't get the most out of Kotlin.
Expected code
I have confirmed that this code can actually be rewritten. You can try it out using this library: https://github.com/sya-ri/commandapi-kotlin-improved
In my library, the Kotlin DSL for CommandAPICommand is not supported, but we should be able to rewrite it similarly.
Extra details
If necessary, I would like to submit a PR for the changes made by my library 😉
The text was updated successfully, but these errors were encountered: