Allow required arguments after optional arguments (Linking Edition) by DerEchtePilz · Pull Request #395 · CommandAPI/CommandAPI

DerEchtePilz · 2023-01-05T23:27:47Z

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

…plementation

DerEchtePilz · 2023-01-06T01:33:53Z

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

…ents() into AbstractArgument#combineWith()

… to the combined argument

…oRegister()

…time

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 #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 :).

commandapi-core/src/main/java/dev/jorel/commandapi/arguments/AbstractArgument.java

commandapi-core/src/main/java/dev/jorel/commandapi/AbstractCommandAPICommand.java

commandapi-documentation-code/src/main/java/dev/jorel/commandapi/examples/java/Examples.java

willkroboth · 2023-01-07T01:15:35Z

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 · 2023-01-07T11:26:14Z

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.

commandapi-core/src/main/java/dev/jorel/commandapi/AbstractCommandAPICommand.java

commandapi-core/src/main/java/dev/jorel/commandapi/arguments/AbstractArgument.java

DerEchtePilz · 2023-01-07T13:50:55Z

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 · 2023-01-07T14:17:55Z

.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.

…ptionalArgumentException

DerEchtePilz · 2023-01-07T14:32:15Z

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

willkroboth · 2023-01-07T14:45:13Z

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 · 2023-01-07T14:50:11Z

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 · 2023-03-08T10:25:11Z

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.

DerEchtePilz added 2 commits January 6, 2023 00:09

allowing required arguments to come after optional arguments

d143de1

change AbstractArgument#linkArguments to accept an actual argument in…

09d1bd2

…plementation

DerEchtePilz marked this pull request as draft January 6, 2023 01:41

DerEchtePilz added 2 commits January 6, 2023 10:41

Remove imports from CommandAPIMain. Rename AbstractArgument#linkArgum…

5567697

…ents() into AbstractArgument#combineWith()

make AbstractArgument#combineWith() copy requirements and permissions…

8aecdfe

… to the combined argument

DerEchtePilz marked this pull request as ready for review January 6, 2023 10:30

DerEchtePilz and others added 4 commits January 6, 2023 22:41

add documentation for the combineWith method

e206166

Merge branch 'JorelAli:dev/dev' into dev/dev

5ad5d9e

fixing argument generation in AbstractCommandAPICommand#getArgumentsT…

d340cd3

…oRegister()

fix AbstractCommandAPICommand#getArgumentsToRegister() for real this …

9b5b663

…time

willkroboth requested changes Jan 7, 2023

View reviewed changes

DerEchtePilz added 4 commits January 7, 2023 12:02

handle chained calls of AbstractArgument#combineWith() correctly

a598955

make requirements and permissions copy values correctly

b5f0ee9

removes an unused import in AbstractCommandAPICommand

7e54622

improve a message for the combined arguments examples

162fd76

improve JavaDocs for AbstractArgument#combineWith()

858097f

willkroboth requested changes Jan 7, 2023

View reviewed changes

optimize AbstractCommandAPICommand#getArgumentsToRegister(), modify O…

6f0878e

…ptionalArgumentException

remove an unnecessary check in AbstractCommandAPICommand#register

7a90e77

willkroboth approved these changes Jan 7, 2023

View reviewed changes

improve JavaDocs of AbstractArgument#combineWith()

af681d7

JorelAli self-assigned this Jan 22, 2023

JorelAli mentioned this pull request Feb 3, 2023

Allow required arguments after optional arguments #394

Closed

3 tasks

DerEchtePilz closed this Feb 3, 2023

DerEchtePilz force-pushed the dev/dev branch from 9838b26 to 7ec80bb Compare February 3, 2023 14:05

Merge JorelAli:dev/dev into dev/dev

2577d57

DerEchtePilz reopened this Feb 3, 2023

DerEchtePilz closed this Feb 23, 2023

DerEchtePilz force-pushed the dev/dev branch from 2577d57 to 8f75713 Compare February 23, 2023 17:04

Merge 'DerEchtePilz:dev/dev' into 'dev/dev'

b287a86

DerEchtePilz reopened this Feb 23, 2023

JorelAli requested changes Mar 3, 2023

View reviewed changes

JorelAli merged commit bd1ed81 into CommandAPI:dev/dev Mar 8, 2023

Uh oh!

Conversation

DerEchtePilz commented Jan 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DerEchtePilz commented Jan 6, 2023

Uh oh!

willkroboth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

willkroboth commented Jan 7, 2023

Uh oh!

DerEchtePilz commented Jan 7, 2023

Uh oh!

willkroboth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DerEchtePilz commented Jan 7, 2023

Uh oh!

willkroboth commented Jan 7, 2023

Uh oh!

DerEchtePilz commented Jan 7, 2023

Uh oh!

willkroboth commented Jan 7, 2023

Uh oh!

DerEchtePilz commented Jan 7, 2023

Uh oh!

willkroboth left a comment

Choose a reason for hiding this comment

Uh oh!

JorelAli left a comment

Choose a reason for hiding this comment

Uh oh!

JorelAli commented Mar 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DerEchtePilz commented Jan 5, 2023 •

edited

Loading