Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CommandAPI command executors should have an overload for arguments accessible via a map #360

Closed
JorelAli opened this issue Nov 11, 2022 · 3 comments
Labels
enhancement New feature or request implemented for next release This has been implemented in the current dev build for the next public release
Milestone

Comments

@JorelAli
Copy link
Owner

Description

CommandAPI command executors currently only consist of a command sender and an array of arguments:

(sender, args) -> {
    // ...
}

It is useful to be able to access arguments by node name, rather than by index - especially in the case of using optional arguments:

(CommandSender sender, Map<String, Object> args) -> {
    String input = (String) args.get("blah");
    int amount = (int) args.getOrDefault("amount", 1);
}

It would be ideal to implement an overload for the various executes() methods that instead of accepting a (sender, args), it can instead accept a generalized (executioninfo) which would allow the CommandAPI to be more easily extensible in the future and provide users with a more accessible API.

Expected code

This could be implemented using a record instead:

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

Generic structure for a player execution, using the generic type to enforce the return type of sender() as as Player object:

@FunctionalInterface
public interface PlayerExecution {

    public void run(ExecutionInfo<Player> info);

}

The implementation of argsMap would be as simple as tweaking the implementation of CommandAPIHandler.argsToObjectArr(), and shoving them into a LinkedHashMap. We'll use Map as the main interface for argsMap() for simplicity, but mention in JavaDocs that the implementation is declared in order (so can be iterated upon if users so desire...)

Extra details

No response

@JorelAli JorelAli added the enhancement New feature or request label Nov 11, 2022
DerEchtePilz added a commit to DerEchtePilz/CommandAPI that referenced this issue Nov 24, 2022
@willkroboth
Copy link
Collaborator

For those who want extra reading on the context of this suggestion, it started as a comment in discord

@Sculas
Copy link

Sculas commented Nov 25, 2022

My idea was: (as discussed on Discord)

new CommandAPICommand("suggestion")
    .withArguments(new ItemStackArgument("item"))
    .executesPlayer(info -> {
        ItemStack possibilityOne = info.args().get(0); // @Nullable public <T> T get(int index)
        ItemStack possibilityTwo = info.args().get("item"); // @Nullable public <T> T get(String identifier)
        // ^ @Nullable because the developer should handle these cases, it shouldn't throw a "hidden" exception

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

This merges args and argsMap into one CommandArgs, containing the 2 overloading methods shown above.
This also removes the need for cloning the argsMap.values() into args.
In Kotlin, the info.args().get(...) methods would be simplified to info.args[...], which is much cleaner.

The implementation code I had in mind for this was:
Note: none of this has been checked in an IDE and is all written on GitHub, Please interpret this as pseudocode.

public record ExecutionInfo<Sender extends CommandSender> (
    Sender sender,
    CommandArgs args
) {}

public class CommandArgs {
    private final Map<String, Object> args; // IMPL: use a map that can be indexed

    // ... constructor ...

    @Nullable
    public <T> T get(int index) {
        /* IMPL: get value from the map by index */
    }

    @Nullable
    public <T> T get(String identifier) {
        return (T) args.get(identifier); // casting null to T is a no-op
    }
}

@JorelAli JorelAli added this to the 9.0.0 milestone Dec 10, 2022
JorelAli pushed a commit that referenced this issue Dec 17, 2022
* adding #360

* provide ExecutionInfo with a record

* fixing JavaDocs

* fixing some things

* adding a CommandArguments class to hold provided arguments

* remove method parameters, fix documentation code to match new syntax

* fix Kotlin DSL to match new syntax

* fixing Converter

* fix some more documentation issues

* fixing some annotation examples to match new syntax

* fix annotation generation to match new syntax

* handle String[] for converted commands correctly

* parse arguments only once

* Make argsToObjectArr return a CommandArguments object. Change name of argsToObjectArr to argsToCommandArgs

* fixing generateCommand method

* fix Brigadier#parseArguments method

* add Javadocs to ExecutionInfo

* Remove test command thingy from CommandAPIMain. Add Javadocs to CommandArguments

* clean up after merge

* fixing a method call in Brigadier#parseArguments

* fixing CommandTree DSL

* optimizing some imports

* fixing some formatting issues

* fixing documentation example code

* fix commands not being executable

* fixing build error
@JorelAli JorelAli added the implemented for next release This has been implemented in the current dev build for the next public release label Dec 17, 2022
@JorelAli
Copy link
Owner Author

Implemented for 9.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request implemented for next release This has been implemented in the current dev build for the next public release
Projects
Archived in project
Development

No branches or pull requests

3 participants