Type-safe Kotlin DSL

Description

The current Kotlin DSL requires casts, so it is not type-safe. We can't get the most out of Kotlin.

commandTree("sendmessageto") {
    playerArgument("player") {
        greedyStringArgument("msg") {
            anyExecutor { _, args ->
                val player: Player = args["player"] as Player
                val message: String = args["msg"] as String
                player.sendMessage(message)
            }
        }
    }
}

commandAPICommand("sendmessageto") {
    playerArgument("player")
    greedyStringArgument("msg")
    anyExecutor { _, args ->
        val player: Player = args["player"] as Player
        val message: String = args["msg"] as String
        player.sendMessage(message)
    }
}

from: Defining a simple message command

Expected code

commandTree("sendmessageto") {
    playerArgument("player") { getPlayer ->
        greedyStringArgument("msg") { getMessage ->
            anyExecutor { _, args ->
                val player = getPlayer(args) // Returns Player
                val message = getMessage(args) // Returns String
                player.sendMessage(message)
            }
        }
    }
}

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 😉

Hmm, interesting.
If you choose to open a PR, please note that this should be an additional option, not a replacement.
Also, this should also be supported in the commandAPICommand structure without the need to redefine how that looks.

@DerEchtePilz

@DerEchtePilz Thank you for your interest.

We will need to make a breaking change to "optional".

Currently, specifying optional = true for argument makes it nullable, and specifying false makes it not.

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

@sya-ri
Yeah, adding the Optional part to the method doesn't work.
When adding optional arguments, it was discussed whether we want a separate method for optional arguments or if we want it as a parameter which by default is set to false.
We obviously settled on the second option and the reason was that doesn't require twice as many methods.
It would be required to keep the optional parameter.

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 CommandTree - you would rather handle it with branching which is why adding methods with the Optional part in the name is unncessary work. It's going to be removed anyway.

Currently, specifying optional = true for argument makes it nullable, and specifying false makes it not.

This will also never change. Optional arguments just have the risk of not being present which is why we return null if they aren't given.

We obviously settled on the second option and the reason was that doesn't require twice as many methods.

Yes, that's exactly right 😢

https://github.com/JorelAli/CommandAPI/blob/3dafcf8ab2a0bf5b407930cf12f67ea4b3ade0c6/commandapi-kotlin/commandapi-bukkit-kotlin/src/main/kotlin/dev/jorel/commandapi/kotlindsl/CommandAPICommandDSL.kt#L114-L117
https://github.com/JorelAli/CommandAPI/blob/3dafcf8ab2a0bf5b407930cf12f67ea4b3ade0c6/commandapi-kotlin/commandapi-bukkit-kotlin/src/main/kotlin/dev/jorel/commandapi/kotlindsl/CommandTreeDSL.kt#L107-L110
https://github.com/JorelAli/CommandAPI/blob/3dafcf8ab2a0bf5b407930cf12f67ea4b3ade0c6/commandapi-kotlin/commandapi-bukkit-kotlin/src/main/kotlin/dev/jorel/commandapi/kotlindsl/CommandTreeDSL.kt#L212-L215

I would like to take this opportunity to remove the deprecated methods.
It's a bit strange to add optional methods to deprecated methods and then deprecate it.

https://github.com/JorelAli/CommandAPI/blob/3dafcf8ab2a0bf5b407930cf12f67ea4b3ade0c6/commandapi-kotlin/commandapi-bukkit-kotlin/src/main/kotlin/dev/jorel/commandapi/kotlindsl/CommandAPICommandDSL.kt#L11-L12
https://github.com/JorelAli/CommandAPI/blob/3dafcf8ab2a0bf5b407930cf12f67ea4b3ade0c6/commandapi-kotlin/commandapi-bukkit-kotlin/src/main/kotlin/dev/jorel/commandapi/kotlindsl/CommandTreeDSL.kt#L9-L10
https://github.com/JorelAli/CommandAPI/blob/3dafcf8ab2a0bf5b407930cf12f67ea4b3ade0c6/commandapi-kotlin/commandapi-bukkit-kotlin/src/main/kotlin/dev/jorel/commandapi/kotlindsl/CommandTreeDSL.kt#L222-L225

I would to leave deprecated methods that are not related to arguments.

What do you think?

I hate that MultiLiteralArgument mess...
Luckily this is already resolved and I am removing deprecated methods in #493

A bit of history about that MultiLiteralArgument mess. I actually added the node name to MultiLiteralArguments but couldn't directly remove the existing methods obviously.
That's why I initially deprecated one method in the DSL and added the (then not deprecated) alternative.
I then fixed that mess and now there are two deprecated methods.

But yeah, those deprecations are already in the process of being removed, we just can't do it until 10.0.0

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 AbstractCommandTree and AbstractArgumentTree:

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 CommandArguments#getUnchecked) from the developer's view.

With AbstractCommandAPICommand, if I add these methods:

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 OptionalArgumentProvider class, which looks like this:

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 Function<CommandArguments, T> for this case where there are multiple ways that the developer might choose to access the optional argument.

Implement testing IntegerRangeArgument NMS independently

During that, we also kinda decided that optional arguments don't really make sense in a CommandTree - you would rather handle it with branching which is why adding methods with the Optional part in the name is unncessary work. It's going to be removed anyway.

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

and it seems to make sense to use Optional Argument in 'thenNested()' after implementing #529?

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 withOptionalArguments method like CommandAPICommands to control when optional arguments could appear to avoid ambiguous cases:

https://github.com/JorelAli/CommandAPI/blob/9d54938aa36162b462e5a722e8b8bbd3b8d0990d/commandapi-core/src/main/java/dev/jorel/commandapi/AbstractCommandTree.java#L61-L64

But talking about this in Discord we thought that this was probably better handled directly by the user. I think thenNested is actually pretty helpful for doing that. For example, these commands are equivalent:

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 thenNested may be clearer about the command structure than the CommandAPICommand imo :P, and you get the flexibility of more branches in the tree. But yeah, I think sticking with just branches in a CommandTree is a good way to clearly handle "optional" arguments without any ambiguity, especially with thenNested.

I think sticking with just branches in a CommandTree is a good way to clearly handle "optional" arguments without any ambiguity, especially with thenNested.

yeah, kotlin dsl ver even more better:

commandTree("test") {
    nested {
        some args...
        literalArgument("aaa", this::handle)
    }
}

Navigation Menu

Repository navigation

Type-safe Kotlin DSL #544

Description

Expected code

Extra details

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Type-safe Kotlin DSL #544

Description

Description

Expected code

Extra details

Activity

DerEchtePilz commented on Apr 13, 2024

sya-ri commented on Apr 13, 2024

DerEchtePilz commented on Apr 13, 2024

sya-ri commented on Apr 13, 2024

DerEchtePilz commented on Apr 13, 2024

sya-ri commented on Apr 14, 2024

DerEchtePilz commented on Apr 14, 2024

sya-ri commented on Apr 14, 2024

willkroboth commented on Apr 14, 2024

MC-XiaoHei commented on Nov 14, 2024

willkroboth commented on Nov 14, 2024

MC-XiaoHei commented on Nov 15, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions