Allow required arguments after optional arguments

[willkroboth]

This PR is me implementing what I was trying to explain here in discord so I can explain it better.

Currently, if you put a required argument after an optional argument, you get an OptionalArgumentException. However, I think this example command makes a good case for changing that:

new CommandAPICommand("rate")
	.withArguments(
		new StringArgument("topic").setOptional(true),
		new IntegerArgument("rating", 0, 10),
		new PlayerArgument("target").setOptional(true)
	)
	.executes(info -> {
		String topic = (String) info.args().get("topic");
		if(topic == null) {
			// Honestly not sure how to justify topic being optional here,
			// causing there to be a required argument after an optional one,
			// but I'm sure an example exists, right?
			info.sender().sendMessage("You didn't give a rating");
			return;
		}

		// We know this is not null because rating is required if topic is given
		int rating = (int) info.args().get("rating");

		// The target player is optional, so give it a default here
		CommandSender target = (CommandSender) info.args().getOrDefault("target", info.sender());

		target.sendMessage("Your " + topic + " was rated: " + rating + "/10");
	})
	.register();

This command can be executed in the following ways:

/rate
/rate <topic> <rating>
/rate <topic> <rating> <target>

Putting the required <rating> argument after the optional <topic> argument indicates that if a topic is given, a rating is required.

To make this work was actually very simple, just a small change to the old system. AbstractCommandAPICommand#getArgumentsToRegister now looks like this:

private List<Argument[]> getArgumentsToRegister(Argument[] argumentsArray) {
	List<Argument[]> argumentsToRegister = new ArrayList<>();

	// Create a List of arrays that hold arguments to register split based on where optional arguments are
	List<Argument> currentCommand = new ArrayList<>(argumentsArray.length);
	for(Argument argument : argumentsArray) {
		if(argument.isOptional()) argumentsToRegister.add((Argument[]) currentCommand.toArray(new AbstractArgument[0]));
		currentCommand.add(argument);
	}
	// All arguments are also valid and not added by above loop
	argumentsToRegister.add(argumentsArray);
	return argumentsToRegister;
}

TODO before merge:

• Get the idea approved
• Remove OptionalArgumentException? (I don't think it is used anymore)
• Update documentation
• Update commandapi-bukkit-plugin-test
• Remove the example command from CommandAPIMain

[JorelAli]

// Honestly not sure how to justify topic being optional here,
// causing there to be a required argument after an optional one,
// but I'm sure an example exists, right?

There is actually a use case for this! A help message stating the usage for this command. If a player types /rate, you could feasibly follow up with sending a message to the user stating how to use /rate

[JorelAli]

Migrate this to the documentation before merging, it's a useful example.

[DerEchtePilz]

Writing this here again (this comment comes from this comment on discord):
What if we make this possible by "linking" arguments together?
What I mean by this is that if we made that command /rate with the syntax Will Kroboth suggested, that and are basically one argument when checking optional argument constraints but when we are actually building that argumentsToRegister list it would be possible to register them individually making that syntax possible.

That wouldn't invalidate the OptionalArgumentException and would allow the syntax suggested by you to be implemented.

[DerEchtePilz]

So you would declare your arguments by doing this:

withOptionalArguments(new StringArgument("topic").link(new IntegerArgument("rating", 0, 10))) // "Links" the IntegerArgument to the StringArgument
withOptionalArguments(new PlayerArgument("target"))

That "linking" makes it so that when checking the optional argument constraints the required IntegerArgument is ignored.
Also this would make it so that the IntegerArgument is required when the StringArgument is given.
When building argumentsToRegister we would simply check if an optional argument has linked arguments and append them to the list.
Of course it wouldn't make sense to give required arguments a linked arguments since they are required anyway.

[DerEchtePilz]

Also, how would this system handle this:

List<ThingToTestCommandAPIThings> list = List.of(
    new ThingToTestCommandAPIThings(),
    new ThingToTestCommandAPIThings().setOptional(true),
    new ThingToTestCommandAPIThings().setOptional(true),
    new ThingToTestCommandAPIThings().setOptional(true),
    new ThingToTestCommandAPIThings().setOptional(false)
);

This was my way of testing that system without needing to import something and I am able to test this in a plain Java project.
Your new system generates this:

[ThingToTestCommandAPIThings{optional=false}]
[ThingToTestCommandAPIThings{optional=false}, ThingToTestCommandAPIThings{optional=true}]
[ThingToTestCommandAPIThings{optional=false}, ThingToTestCommandAPIThings{optional=true}, ThingToTestCommandAPIThings{optional=true}]
[ThingToTestCommandAPIThings{optional=false}, ThingToTestCommandAPIThings{optional=true}, ThingToTestCommandAPIThings{optional=true}, ThingToTestCommandAPIThings{optional=true}, ThingToTestCommandAPIThings{optional=false}]

However, I would expect it to only generate one result because I have a required argument at the end.
This was also part of the decision why I decided to even bother with those optional argument constraints.
In my opinion (and please don't take it personal, because it really isn't meant to offend anyone) we should stick with the system I implemented since there are now many more possibilities because optional arguments are implemented.

[willkroboth]

Also, how would this system handle this:

List<ThingToTestCommandAPIThings> list = List.of(
    new ThingToTestCommandAPIThings(),
    new ThingToTestCommandAPIThings().setOptional(true),
    new ThingToTestCommandAPIThings().setOptional(true),
    new ThingToTestCommandAPIThings().setOptional(true),
    new ThingToTestCommandAPIThings().setOptional(false)
);

This was my way of testing that system without needing to import something and I am able to test this in a plain Java project. Your new system generates this:

[ThingToTestCommandAPIThings{optional=false}]
[ThingToTestCommandAPIThings{optional=false}, ThingToTestCommandAPIThings{optional=true}]
[ThingToTestCommandAPIThings{optional=false}, ThingToTestCommandAPIThings{optional=true}, ThingToTestCommandAPIThings{optional=true}]
[ThingToTestCommandAPIThings{optional=false}, ThingToTestCommandAPIThings{optional=true}, ThingToTestCommandAPIThings{optional=true}, ThingToTestCommandAPIThings{optional=true}, ThingToTestCommandAPIThings{optional=false}]

Yeah, the output there is what I expected. Simplifying that a bit so its easier to read and talk about, the input is:

A, B (optional), C (optional), D (optional), E

And the output is:

A
A, B
A, B, C
A, B, C, D, E

So, I guess calling a required argument that is after an optional argument "required" isn't quite right, since you don't need to always include it (E is "required", but not present in all of the commands). That is necessary to avoid ambiguity while still being useful. With the example, if E always had to be included, you could either add it to the end of every command and get:

A, E
A, B, E
A, B, C, E
A, B, C, D, E

or require all arguments before it, basically ignoring their optional status. So yeah, you are correct that if E (the last argument) was "required" that wouldn't really work.

What my system is doing instead is actually pretty much linking the arguments. So E isn't strictly fully required, it is required if D is present. You could say E is linked to D, but without an explicitly linked argument structure, more implicitly by the fact that E is not optional (required) and after D.

I hope that explains why the output was correct to be like that? Maybe there is a better word than required for arguments like E and rating that would make this property clearer.

[DerEchtePilz]

Yeah, the output there is what I expected. Simplifying that a bit so its easier to read and talk about, the input is:

A, B (optional), C (optional), D (optional), E

And the output is:

A
A, B
A, B, C
A, B, C, D, E

So, I guess calling a required argument that is after an optional argument "required" isn't quite right, since you don't need to always include it (E is "required", but not present in all of the commands). That is necessary to avoid ambiguity while still being useful. With the example, if E always had to be included, you could either add it to the end of every command and get:

A, E
A, B, E
A, B, C, E
A, B, C, D, E

or require all arguments before it, basically ignoring their optional status. So yeah, you are correct that if E (the last argument) was "required" that wouldn't really work.

What my system is doing instead is actually pretty much linking the arguments. So E isn't strictly fully required, it is required if D is present. You could say E is linked to D, but without an explicitly linked argument structure, more implicitly by the fact that E is not optional (required) and after D.

I hope that explains why the output was correct to be like that? Maybe there is a better word than required for arguments like E and rating that would make this property clearer.

Well, what I think is this:

• First option: Keep my system. The reason for that is that we can pretty much do everything we want with a combination of subcommands and optional arguments
• Second option: Append E to every argument making it indeed required. We have the ability to access arguments by using their node names. So what we could do is throw the OptionalArgumentException if we have same node names on at least two arguments

[willkroboth]

Note the example command:

new CommandAPICommand("rate")
	.withArguments(
		new StringArgument("topic").setOptional(true),
		new IntegerArgument("rating", 0, 10),
		new PlayerArgument("target").setOptional(true)
	)
	.executes(info -> {
		String topic = (String) info.args().get("topic");
		if(topic == null) {
			info.sender().sendMessage("You didn't give a rating");
			return;
		}

		int rating = (int) info.args().get("rating");
		CommandSender target = (CommandSender) info.args().getOrDefault("target", info.sender());

		target.sendMessage("Your " + topic + " was rated: " + rating + "/10");
	})
	.register();

is equivalent to this CommandTree:

new CommandTree("rate")
	.executes((info) -> info.sender().sendMessage("You didn't give a rating"))
	.then(
		new StringArgument("topic").then(
			new IntegerArgument("rating")
				.executes((info) -> {
					String topic = (String) info.args().get("topic");
					int rating = (int) info.args().get("rating");

					info.sender().sendMessage("Your " + topic + " was rated: " + rating + "/10");
				})
				.then(
					new PlayerArgument("target")
						.executes((info) -> {
							String topic = (String) info.args().get("topic");
							int rating = (int) info.args().get("rating");
							Player target = (Player) info.args().get("target");

							target.sendMessage("Your " + topic + " was rated: " + rating + "/10");
						})
				)
		)
	)
	.register();

I would consider the example command simpler and easier to understand, making this an example of when this sort of interaction between optional and required arguments would be useful.

[JorelAli]

I think there's a bit of merit in the "linked" concept and I think it resolves this ambiguity in the declaration of the /rate command in the original post.

The claim is that the /rate command declared like this:

new CommandAPICommand("rate")
    .withArguments(
        new StringArgument("topic").setOptional(true),
        new IntegerArgument("rating", 0, 10),
        new PlayerArgument("target").setOptional(true)
    )

leads to this list of possible commands:

/rate
/rate <topic> <rating>
/rate <topic> <rating> <target>

However, the ambiguity here is that this could also be interpreted as a valid command:

/rate <rating>
/rate <rating> <target>

The requirement that "rating" must follow "topic" is 'lost' in this (the original) implementation. By 'lost', it may be inferred by the implementation of the CommandAPI, but as someone reading the code, it doesn't state this. The idea of having linked arguments addresses this - one (or more) argument(s) that must follow another argument and the whole thing can be classed as "an optional argument".

---

I think the idea of some sort of "combined" argument makes sense. Say we have something like this:

class CombinedArgument {
    CombinedArgument(Argument... args) { /* Some implementation */ }
}

new CommandAPICommand("rate")
    .withArguments(
        new CombinedArgument(
            new StringArgument("topic").setOptional(true), // [1]
            new IntegerArgument("rating", 0, 10)
        ).setOptional(true),
        new PlayerArgument("target").setOptional(true)
    )

In this example, it's absolutely clear that "topic and rating go together". All properties of the inner arguments (e.g. [1] or setting permissions/requirements) are ignored, they must be implemented by the parent argument (the "combined argument"). We can then flag the whole thing as "optional", but it's 100% clear what's going on.

The reason I think this works better than the original "linked" argument is because it avoids conflicting permissions on arguments - if you have a permission set on rating but not on topic, then the user ends up with a half-baked command. Sure, you can implement this with linked arguments, but it's hard for a developer to figure out "which of the two arguments do I have to put my permission on to make it apply to both of them?"

---

Briefly touching on some of the earlier conversation, in particular this example:

A, E
A, B, E
A, B, C, E
A, B, C, D, E

This was one of the primary reasons that the implementation of optional arguments stood around for 2 years. I initially wanted to have this sort of behaviour, but upon seeing the implementation by @DerEchtePilz, I found that it became a lot easier to interpret and design commands without having to worry about this. (Of course, being able to access arguments by their node name makes this significantly easier to deal with as well!)

[JorelAli]

I think we're going to sunset this PR in favour of #395. Linked/combined arguments seems to flow better and have less ambiguity.