Allow required arguments after optional arguments (Linking Edition)

[DerEchtePilz]

Due to some discussions on discord, I openend this PR to present yet another method of allowing required arguments after optional arguments.
This doesn't get rid of the OptionalArgumentException and doesn't change the original system introduced in #393 too much.

Here is the rate command from #394 using linking:

new CommandAPICommand("rate")
	.withArguments(
		new StringArgument("topic").setOptional(true).combineWith(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 works exactly the same as the rate command in #394 but it uses linking.
The combineWith() method takes in any number of arguments and will lead to the IntegerArgument being only required if the StringArgument is given.
The PlayerArgument behaves like you would it expect it to behave.

Things left to do:

• Test this. Due to the changes made to the if-statement, I am not sure if this does what I want it to
• Possibly write documentation?
• Fix some types
• Maybe rename the linkArguments method to combineArguments (coming from here)
• Make combineArguments copy permissions and requirements to the added arguments (coming from here)
• Remove some imports from CommandAPIMain

[DerEchtePilz]

I just thought about this and I will rename the linkArguments method into combineArguments

[willkroboth]

Overall, looks good, just a couple of bugs to iron out.

I'm still not sure that combining arguments adds enough value to be worth the extra complexity.
Pros:

• Clearer that the child argument is required when following the parent argument
• Can attach the same permission and requirement to combined arguments (Actually, I'm not sure if this is useful. If the user doesn't have permission for the parent argument, they can't type it in. If they can't type the parent argument, the child argument is inaccessible regardless of its permission.)

Cons:

• More complexity
  • Need to unwrap combined arguments (and handle layered combined arguments)
  • Need to enforce the OptionalArgumentException (relative to Allow required arguments after optional arguments #394)

Commands written with #394 are almost the same as with this system. For example, if I did the following with #394:

.withArguments(
	new StringArgument("A").setOptional(true),
	new StringArgument("B"),
	new StringArgument("C"),
	new StringArgument("D").setOptional(true),
	new StringArgument("E"),
	new StringArgument("F").setOptional(true)
)

The equivalent here would be:

.withArguments(
	new StringArgument("A").setOptional(true).combineWith(
		new StringArgument("B"),
		new StringArgument("C")
	),
	new StringArgument("D").setOptional(true).combineWith(
		new StringArgument("E")
	),
	new StringArgument("F").setOptional(true)
)

All I see is that arguments B, C, and E are indented an extra layer. I'm not sure if that's worth the extra behind-the-scenes complexity and headaches I see in the more complicated AbstractCommandAPICommand#getArgumentsToRegister method.

If the general consensus is that this PR makes developer code clearer though, I think it'll work out :).

[willkroboth]

Also, I'm not sure if this is bad, but if you combine arguments the list returned by CommandAPICommand#getArguments is kinda wrong.
For example, these arguments:

.withArguments(
	new StringArgument("A").setOptional(true).combineWith(
		new StringArgument("B"),
		new StringArgument("C")
	),
	new StringArgument("D").setOptional(true).combineWith(
		new StringArgument("E")
	),
	new StringArgument("F").setOptional(true)
)

Return this List:

[StringArgument("A"), StringArgument("D"), StringArgument("F")]

The length of this list of 3, even though there actually 6 arguments on this command. Without inspecting the combined arguments (kinda tricky), you wouldn't know about the arguments B, C, or E.

Again, I don't know if this is bad since if a developer combined arguments they should know what has happened, but it's something to note.

[DerEchtePilz]

Also, I'm not sure if this is bad, but if you combine arguments the list returned by CommandAPICommand#getArguments is kinda wrong. For example, these arguments:

.withArguments(
	new StringArgument("A").setOptional(true).combineWith(
		new StringArgument("B"),
		new StringArgument("C")
	),
	new StringArgument("D").setOptional(true).combineWith(
		new StringArgument("E")
	),
	new StringArgument("F").setOptional(true)
)

Return this List:

[StringArgument("A"), StringArgument("D"), StringArgument("F")]

The length of this list of 3, even though there actually 6 arguments on this command. Without inspecting the combined arguments (kinda tricky), you wouldn't know about the arguments B, C, or E.

Again, I don't know if this is bad since if a developer combined arguments they should know what has happened, but it's something to note.

I want to say, that it may not be a bad thing. The method's JavaDocs state that combined arguments are ignored until the arguments to register are built.

[willkroboth]

Mostly some ways to optimize the getArgumentsToRegister method.

Note on permission (and requirement) sharing: I don't think it's actually helpful (referenced by another comment)

For example, doing this:

.withArguments(
	new StringArgument("base").withPermission("permission").combineWith(
		new StringArgument("child")
	)
)

Arguments base and child both have the permission permission. However, since child always comes after base, this doesn't do anything in practice. If base had the permission, and child didn't the outcome would be the same.

If the user doesn't have permission, they won't be allowed to type the base argument, and child is inaccessible anyways.
If the user does have permission, they can type the base argument. We know the user has the permission permission at this point, so it doesn't matter if child requires that permission as well or just has no permission.
The same logic applies to requirements and requirement sharing.

This is to say, permission sharing isn't an advantage of combining arguments since it basically does nothing.

[DerEchtePilz]

Mostly some ways to optimize the getArgumentsToRegister method.

Note on permission (and requirement) sharing: I don't think it's actually helpful (referenced by another comment)

For example, doing this:

.withArguments(
	new StringArgument("base").withPermission("permission").combineWith(
		new StringArgument("child")
	)
)

Arguments base and child both have the permission permission. However, since child always comes after base, this doesn't do anything in practice. If base had the permission, and child didn't the outcome would be the same.

If the user doesn't have permission, they won't be allowed to type the base argument, and child is inaccessible anyways. If the user does have permission, they can type the base argument. We know the user has the permission permission at this point, so it doesn't matter if child requires that permission as well or just has no permission. The same logic applies to requirements and requirement sharing.

This is to say, permission sharing isn't an advantage of combining arguments since it basically does nothing.

You miss the point that it also overwrites permissions and requirements on "child".
If child has a permission attached to it and base doesn't, you end up with a command that always is invalid.
This was also mentioned on your PR #394
So yes, that permission sharing does indeed something useful

[willkroboth]

.withArguments(
	new StringArgument("base").withPermission("permission").combineWith(
		new StringArgument("child").withPermission("otherPermission")
	)
)

Yeah, I guess in the above case, base and child will both end up with the "permission" permission. However, while allowing child to keep "otherPermission" would mean that users would need both "permission" and "otherPermission" to run this command, I think that should be up to developers to decide.

I think the above example indicates that the developer wants base to have "permission" and child to have "otherPermission". The reference to "otherPermission" just kinda gets deleted here without warning, which might be confusing. If they want to 'break' their command like that, they probably have a good reason? If they didn't want that, they should just not apply a permission to child.

[DerEchtePilz]

Now, I've decided to implement your suggestion for the getArgumentsToRegister method since I really couldn't come up with something better.

[willkroboth]

Okay, if you went with that, something to note is that I wrote it before I knew the case when there are no optional arguments was handled outside in AbstractCommandAPICommand#register. So basically now, the argumentsToRegister list will always contain some arguments, and the check for argumentsToRegister.isEmpty() on line 278 will always be false.

[DerEchtePilz]

Okay, if you went with that, something to note is that I wrote it before I knew the case when there are no optional arguments was handled outside in AbstractCommandAPICommand#register. So basically now, the argumentsToRegister list will always contain some arguments, and the check for argumentsToRegister.isEmpty() on line 278 will always be false.

Yeah, I guess this way also makes a little bit more sense since the method is called getArgumentsToRegister

[willkroboth]

The code looks good to me 👍. The concept I'm still not sure about, but I think it'll work out if Skepter approves it too.

[JorelAli]

This wasn't a requirement when this PR was originally raised, but it'll be good to follow suit with new standards. I'd like some units tests for this PR please.

[JorelAli]

Bypassing the requirement for tests, we'll implement them later, but make sure they get implemented before 9.0.0's release. If any future issues arise during testing, we'll find them and we'll fix them.