adding #360 #369

DerEchtePilz · 2022-11-24T21:50:27Z

Hello!

It's me again.

I opened this PR so you can decide if you want to have it in 8.6.0 or later (or if you want me to change/add anything).

DerEchtePilz · 2022-11-24T21:56:14Z

Also, just wanted to mention that this currently is untested which will be done until noon on Saturday.

DerEchtePilz · 2022-11-24T22:43:27Z

As requested on discord, I will now add some code examples and explain my changes a little more.

1. Internal changes

To be able to implement this without having to use if-statements (in fact, I didn't use a single one), I changed the executeWith method in the IExecutorTyped interface to now also take in a Map<String, Object>. This map is filled with content in CommandAPIHandler#generateCommand.

1.1 Interfaces and `executes...`

Before I made this PR, each executor had two interfaces: ...CommandExecutor and ...ResultingCommandExecutor.
Now, two more interfaces have been added to support the use of the map:

...ExecutionInfo
...ResultingExecutionInfo

The ...ExecutionInfo and ...ResultingExecutionInfo interfaces both have one method:

...ExecutionInfo:

void run(BlockCommandSender sender, Object[] args, Map<String, Object> argsMap) throws WrapperCommandSyntaxException;

...ResultingExecutionInfo:

int run(BlockCommandSender sender, Object[] args, Map<String, Object> argsMap) throws WrapperCommandSyntaxException;

Those two interfaces are now also available through the use of the .executes... methods.
The already existing .executes... methods have now been overloaded to also accept the new ExecutionInfo interfaces:

public T executesPlayer(PlayerExecutionInfo info) {
	this.executor.addNormalExecutor(info);
	return (T) this;
}

public T executesPlayer(PlayerResultingExecutionInfo info) {
	this.executor.addResultingExecutor(info);
	return (T) this;
}

Use the new methods in your code

For this section, I will just add some examples I wrote:

new CommandAPICommand("nomap")
	.withArguments(new ItemStackArgument("item"))
	.executesPlayer((player, args) -> {
		// This command does not have access to the map with stores the arguments mapped to their node names
		// We have to get the ItemStack as normal
		ItemStack itemStack = (ItemStack) args[0];
		player.getInventory().addItem(itemStack);
	})
	.register();

new CommandAPICommand("withmap")
	.withArguments(new ItemStackArgument("item"))
	.executesPlayer((player, args, argsMap) -> {
		// This command has access to the map which stores the arguments mapped to their node names
		// Here we can get the ItemStack with two ways:
		// The known way:
		ItemStack possibilityOne = (ItemStack) args[0];

		// Or:
		ItemStack possibilityTwo = (ItemStack) argsMap.get("item");

		player.getInventory().addItem(possibilityOne, possibilityTwo);
	})
	.register();

2. Tests

As written previously, I did not test this yet but I am confident that it should work.
I will do that until Saturday und will update you when I am done.

JorelAli · 2022-11-25T10:19:42Z

Not scheduling for 8.6.0 - too close to the expected release date. I'm more than happy to have this for 9.0.0.

My problem with this implementation is the limited scope for future extensibility. The CommandAPI has been around for just over four years and we're only now adding more accessibly functionality to the .executes() methods - it's quite possible that another four years down the line we may want to add more functionality to our .executes() method. The choice to use another functional interface with three parameters is limiting and leads into questionable design decisions, such as:

What if we want to add a fourth parameter later? What about a fifth? We'll end up with code that gets unnecessarily large:
```
.executesPlayer((player, args, argsMap, somethingElse, someOtherStuff) -> {

})
```

How do we make linters happy? In programming (in general), linters like to complain about unused parameters. Ideally, the CommandAPI should avoid exposing parameters that users are unlikely to used, which is not the case with your implementation:

.executesPlayer((player, args, argsMap) -> {
  // Almost all u
8000
sers that are going to use this method will just use 'player'
  // and argsMap. We end up forcing the user to add this unused 'args' parameter.
  // In Kotlin, we can dismiss this with an underscore, but you can't do that in Java!
})

I think going with a record would be better. It would allow us (CommandAPI development team) to be able to expand the list of accessible functions easily without causing backwards-incompatible changes and without CommandAPI users needing to change their executor implementation. Perhaps something like this would be better?

new CommandAPICommand("withmap")
    .withArguments(new ItemStackArgument("item"))
    .executesPlayer(info -> {
        ItemStack possibilityOne = info.args()[0];             // Access using args()
        ItemStack possibilityTwo = info.argsMap().get("item"); // Access using argsMap()
        Player player = info.sender(); // Access command sender. Using generics, we can make this a Player instead of a CommandSender object.
    })
    .register();

DerEchtePilz · 2022-11-25T11:48:18Z

Yes, I read that record suggestion in #360 and also tried to implement it like that.
I didn‘t manage to do that and decided to go with that solution, knowing it is not ideal.
I am more than happy to add that record, how would I do it in general? Would a Function work?

JorelAli · 2022-11-25T12:09:04Z

I am more than happy to add that record, how would I do it in general? Would a Function work?

I envision something along the lines of creating a generalized record like this:

public record ExecutionInfo<Sender extends CommandSender> (
    Sender sender,
    Object[] args,
    Map<String, Object> argsMap
) {}

I think in the long run it would be safer to create a generic functional interface for our two types rather than using Function<ExecutionInfo, Integer> and Consumer<ExecutionInfo> (especially for Kotlin purposes). Say we add an INormalExecution and an IResultingExecution which follows something like this:

public interface INormalExecution<Sender extends CommandSender> extends IExecutorTyped {

    @Override
    default int executeWith(CommandSender sender, Object[] args) throws WrapperCommandSyntaxException {
        // Some implementation here... either changing the parameters to this function
        // to using ExecutionInfo or something? I didn't think this far ahead
        return 1;
    }

    void run(ExecutionInfo<Sender> info) throws WrapperCommandSyntaxException;

}

Then I think it's safe to say we can add our implementations for each execution type and then link them into Executable. For example, for a normal player execution:

@FunctionalInterface
public interface PlayerCommandExecution extends INormalExecution<Player> {

    void run(ExecutionInfo<Player> player) throws WrapperCommandSyntaxException;

    @Override
    default ExecutorType getType() {
        return ExecutorType.PLAYER;
    }
}

And then linking:

// class dev.jorel.commandapi.Executable {

    public T executesPlayer(PlayerCommandExecution executor) {
        this.executor.addNormalExecutor(executor); // Something to this effect
        return (T) this;
     }
     
// }

If all of the names of "executor" and "execution" and "executable" start getting too complicated, feel free to choose alternatives for the new classes!

In short, the main idea is that we have one centralized record which is easy to update with new changes, and then each new executor implements that.

DerEchtePilz · 2022-11-25T23:07:04Z

This is now tested and should work!

Also, I decided to not include the CommandArgs class which Lucas suggested on discord as I decided to include the Object[] and Map<String, Object> anyway.

JorelAli · 2022-12-13T12:17:30Z

@DerEchtePilz Please update this branch with the latest changes from JorelAli:dev/dev which includes the multi-platform refactor so this can be merged cleanly. For all intents and purposes:

Changes in commandapi-core can be moved to commandapi-bukkit-core
Changes in commandapi-kotlin can be moved to commandapi-kotlin-bukkit
Changes in commandapi-plugin-test can be moved to commandapi-bukkit-plugin-test (don't worry if this doesn't compile, this whole project is in development)

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

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

commandapi-core/src/main/java/dev/jorel/commandapi/executors/CommandArguments.java

commandapi-core/src/main/java/dev/jorel/commandapi/executors/ExecutionInfo.java

DerEchtePilz · 2022-12-13T13:23:59Z

So, what I've decided to do is that I will first resolve those issues right here (because honestly, I kinda do not want to mess anything up) and then I will update this branch.

Can someone please tell me how to update this branch? I've merged conflicting files before but it is important (for me, you and this pull request) that everything goes well.

… argsToObjectArr to argsToCommandArgs

…ndArguments

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

JorelAli

A few minor tid-bits that I noticed during manual testing, but nothing severe enough to prevent merging it over. These minor points should be addressed before 9.0.0 goes out though.

JorelAli · 2022-12-17T19:37:12Z

commandapi-core/src/main/java/dev/jorel/commandapi/executors/AbstractExecutionInfo.java

+	/**
+	 * @return The wrapper type of this command
+	 */
+	WrapperType senderWrapper();


This is exposed to the end user. A comment stating that this is only used internally, and you should be using sender() instead would be nice.

JorelAli · 2022-12-17T19:39:54Z

commandapi-core/src/main/java/dev/jorel/commandapi/executors/CommandArguments.java

+	 * @param nodeName The node name of this argument. This was set when initializing an argument
+	 * @return an argument which has the given node name
+	 */
+	public Object get(String nodeName) {


This method can return null (if nodeName is not found). This should be made known to the end user (by using the javax.annotation.Nullable annotation) and it should be stated in the JavaDocs that this can return null if nodeName is not found.

adding CommandAPI#360

9e519fd

DerEchtePilz added 3 commits November 25, 2022 15:08

provide ExecutionInfo with a record

832128e

fixing JavaDocs

ad5ef54

fixing some things

31d9b61

adding a CommandArguments class to hold provided arguments

e7626e8

DerEchtePilz marked this pull request as draft December 9, 2022 18:11

DerEchtePilz and others added 8 commits December 9, 2022 20:04

remove method parameters, fix documentation code to match new syntax

f23361a

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

bfe45a3

fix Kotlin DSL to match new syntax

45753b1

fixing Converter

0601bfc

fix some more documentation issues

f512772

fixing some annotation examples to match new syntax

d21d50b

fix annotation generation to match new syntax

15f29ff

handle String[] for converted commands correctly

47b86f8

DerEchtePilz marked this pull request as ready for review December 9, 2022 20:40

JorelAli requested changes Dec 13, 2022

View reviewed changes

DerEchtePilz added 6 commits December 13, 2022 14:27

parse arguments only once

92f4e37

Make argsToObjectArr return a CommandArguments object. Change name of…

88b232f

… argsToObjectArr to argsToCommandArgs

fixing generateCommand method

328295e

fix Brigadier#parseArguments method

18eb95d

add Javadocs to ExecutionInfo

d4d07c9

Remove test command thingy from CommandAPIMain. Add Javadocs to Comma…

374ebcc

…ndArguments

JorelAli approved these changes Dec 13, 2022

View reviewed changes

DerEchtePilz added 3 commits December 14, 2022 16:27

updating branch to support new project structure

e6d8cab

clean up after merge

43d3382

fixing a method call in Brigadier#parseArguments

9fbf5a7

DerEchtePilz marked this pull request as draft December 14, 2022 15:47

DerEchtePilz added 2 commits December 14, 2022 16:51

fixing CommandTree DSL

5c01e6a

optimizing some imports

6d4fa44

DerEchtePilz marked this pull request as ready for review December 14, 2022 15:52

DerEchtePilz commented Dec 14, 2022

View reviewed changes

commandapi-core/src/main/java/dev/jorel/commandapi/CommandAPIHandler.java Show resolved Hide resolved

DerEchtePilz and others added 5 commits December 15, 2022 15:24

fixing some formatting issues

9142d85

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

7057cf5

fixing documentation example code

a02a8b8

fix commands not being executable

A36C

eb5bf6e

fixing build error

104391c

JorelAli reviewed Dec 17, 2022

View reviewed changes

JorelAli merged commit 340902c into CommandAPI:dev/dev Dec 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

adding #360 #369

adding #360 #369

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adding #360 #369

adding #360 #369

Uh oh!

Conversation

Uh oh!

Uh oh!

1. Internal changes

1.1 Interfaces and executes...

Use the new methods in your code

2. Tests

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

1.1 Interfaces and `executes...`