For my first discussion point:

Parameters

What are known as CommandElements will become Parameters. Parameters can be implemented like a CommandCallable, and is the low level interface for specialist tasks. Most developers would want to use the high level builders, however.

TokenizedArgs and CommandExecutionContext

Before we continue, it's pertinent to be aware of what these are. Essentially, these are the analogue to CommandArgs and CommandContext in the original system. TokenizedArgs contain the split string that contains the arguments, and these are parsed by the Parameters and entered into the CommandExecutionContext.

The main difference is that these are now interfaces. Sponge will provide implementations for these.

The Parameter Builder, Parameter.Builder

The main builder is exposed as Parameter.builder(), that is, the class Parameter.Builder. To understand how the builder works, first, we need to dive into constituent parts.

ValueParameters and ValueParameterModifiers

Commander exposes two high level parameter types, ValueParameter and ValueParameterModifier. Each parameter can be made up of one ValueParameter and zero or more ValueParameterModifiers.

• ValueParameters are designed to parse the input argument(s) and output a value. This value should be meaningful on its own. Think parseValue in CommandElement, it takes the arguments from TokenizedArgs and returns an Optional with the parsed value(s). They do not tend to add entries to the CommandExecutionContext. ValueParameters also include instructions on how to perform a tab completion, and how to display the usage text for the element.
• ValueParameterModifiers wrap around ValueParameters and allow for some inspection/modification of values. For example, a modifier can let the ValueParameter parse the value, then check to see if there is only one item returned in the context (like GenericArguments.onlyOne(element)). Modifiers can also act before the element is parsed. Modifiers can also manipulate the tab completion and usage strings.

One major thing that you might notice immediately is that these classes don't store the parameter key, but instead are passed it. This allows for these parameters and modifiers to be reused, saving on memory usage - and in the case of standard parameters, some of them can be catalogued in the Sponge catalogue.

Catalogued ValueParameters are found in the CatalogedValueParameters class. Catalogued ValueParameterModifiers are found in the CatalogedValueParameterModifiers class. They are also accessible from ValueParamters and ValueParameterModifiers, which also include some standard ValueParameters and ValueParameterModifiers that do not tend to being single catalogued instances.

ValueParsers, ValueCompleters

ValueParameters are actually made up of two smaller interfaces. ValueParsers parse the value, ValueCompleters define how to tab complete. These become useful in the Parameter builder, and will be discussed later.

Building a Parameter

To build a player parameter named player that you can only get one of, with the permission permissionone required, the current wrapped way of doing it is this:

GenericArguments.requiringPermission(
    GenericArguments.onlyOne(
        GenericArguments.player(Text.of("player")))
, "permissionone");

The difficulty here is that the key can be hard to spot, and the modifiers are not exactly obvious - and with heavily wrapped parameters, this can get very heavy.

My solution is the builder pattern:

Parameter.builder()
    .key(Text.of("player"))
    .valueParser(CatalogedValueParameters.PLAYER)
    .modifiers(CatalogedValueParameterModifiers.ONLY_ONE)
    .permission("permissionone").build();

or,

Parameter.builder()
    .key(Text.of("player"))
    .valueParser(ValueParameters.player())
    .modifiers(ValueParameterModifiers.onlyOne())
    .permission("permissionone").build();

or even, with the inbuilt parameter types:

Parameter.builder()
    .key("player") // The String version of key will do a Text.of(key)
    .player()
    .onlyOne()
    .permission("permissionone")
    .build();

It's potentially a little more verbose, but hopefully easier to get the information out of at a glance. Sponge uses the Builder format, so this also has the benefit of being more familiar to Sponge plugin developers.

If you wanted to use a more dynamic value parameter, say, choices, which is where you can specify arbitary choices, ValueParameters will provide for this:

Parameter.builder()
    .key("player")
    .valueParser(ValueParameters.choices("choice1", "choice2"))
    .build()

or, again:

Parameter.builder()
    .key("player")
    .choices("choice1", "choice2")
    .build()

A similar concept can be applied to modifiers. They exist in the ValueParameterModifiers, or as part of the builder.

Default implemented on ValueParameters is a method, asParameter(Text, ValueParameterModifier...). By specifying the key and modifiers (if any), the builder can be bypassed. So, for a simple player parameter with the only one modifier:

CatalogedValueParameter.PLAYER.asParameter(Text.of("player"), CatalogedValueParameterModifier.ONLY_ONE);

or without:

CatalogedValueParameter.PLAYER.asParameter(Text.of("player"));

Creating Custom ValueParameters

Developers are encouraged to write parameters in one of two ways:

• Parameters that are reusable should be developed as ValueParameters. There are javadocs to help here.
• Parameters that are for one command only could be defined inline in the builder.

The first option should (hopefully) be straightforward, so I'm going to focus on the second here.

The builder has more options:

• parse(ValueParser) - how to parse the element (also accepts ValueParameters)
• onComplete(@Nullable ValueCompleter) - how to respond to a tab completion
• usage(@Nullable BiFunction<Text, CommandSource, Text>) - how to return the usage Text for that element (the function supplies the current usage text and the requesting command source. It returns the new usage text.)

These are all functional interfaces so you can specify these all inline in the builder if you want to. Only the onParse method is required. onComplete defaults to returning an empty list, usage returns the element key.

The following example shows how you can create a very simple parameter inline.

Parameter.builder().key(Text.of("test"))
    .onParse((source, args, context) -> args.next())
    .onComplete(() -> Lists.newArrayList("test1", "test2"))
    .usage((text, source) -> Text.of("[", text, "]"))
    .build();

In fact, you can also do this with a specified ValueParameter, and override the default parameter and the effect of any modifiers with your own completion and usage logic:

Parameter.builder().key(Text.of("test"))
    .valueParameter(CataloguedValueParameter.STRING)
    .onComplete(() -> Lists.newArrayList("test1", "test2"))
    .usage((text, source) -> Text.of("[", text, "]"))
    .build();

This allows for some powerful, and hopefully unintimidating parameter building!

Custom ValueParameterModifiers

Before being able to write an effective ValueParamaterModifier, it's worth knowing how they work. Their behaviour is different based on the action being performed.

• During parsing, the first modifier's onParse method is called, and a ParsingContext is provided to execute the next modifier or ValueParser. To pass control to the next modifier, or the value paser, modifiers should call the ParsingContext#next() method, which will call onParse on the next modifier, or getValue on the value parser. Once the next element has completed executing, the modifier regains control and can do post-parsing checks.
• During completion, each modifier's complete method will be called in turn after the ValueParser has provided the initial List<String>. There is no context to call into the next element, the modifiers are called in the specified order on the parameter builder.
• During usage, each modifier's usage method will be called in turn after the ValueParser has provided the initial Text. Use Text.EMPTY to hide the element. There is no context to call into the next element, the modifiers are called in the specified order on the parameter builder.

So, with that in mind, a sample modifier might look like this:

public class ExampleModifier implements ValueParameterModifier {

    @Override
    public void onParse(Text key, CommandSource source, TokenizedArgs args, CommandExecutionContext context, ParsingContext parsingContext) throws ParameterParseException {
        // At this point, the value has not been parsed, and an arg from TokenizedArgs has not been picked.
        if (source instanceof ConsoleSource) {
            return; // This just does not parse the element
        }

        parsingContext.next(); // Get the value

        // At this point, the value should have been retrieved and in the context.
        if (context.getAll(key).size() > 1) {
            // We're only allowed one.
            throw args.createError("You're only allowed one of those!");
        }
    }

    List<String> complete(CommandSource source, TokenizedArgs args, CommandExecutionContext context, List<String> currentCompletions)
            throws ParameterParseException {
        // Because of the parser, this would never happen, but for illustrative purposes...
        if (source instanceof ConsoleSource) {
            // Remove the current completions
            return Lists.newArrayList(); 
        }
        return currentCompletions;
    }

    default Text getUsage(Text key, CommandSource source, Text currentUsage) {
        if (source instanceof ConsoleSource) {
            // Consoles can't do this, hide the usage for it!
            return Text.EMPTY; 
        }
        return currentUsage;
    }

}

Only the onParse method needs to be implemented - so modifiers can also be expressed inline on the builder, useful for if you specify your own completion and usage text too! So, with this in mind, we can specify a whole builder for this:

Parameter.builder().key(Text.of("test"))
    .valueParameter(CataloguedValueParameter.STRING)
    .onComplete(() -> Lists.newArrayList("test1", "test2")) // No need to alter this!
    .usage((text, source) -> {
        // At this point, the value has not been parsed, and an arg from TokenizedArgs has not been picked.
        if (source instanceof ConsoleSource) {
            return Text.EMPTY;
        }
        return Text.of("[", text, "]");
    })
    .modifiers((key, source, args, context, parsingContext) -> {
        // At this point, the value has not been parsed, and an arg from TokenizedArgs has not been picked.
        if (source instanceof ConsoleSource) {
            return; // This just does not parse the element
        }

        parsingContext.next(); // Get the value

        // At this point, the value should have been retrieved and in the context.
        if (context.getAll(key).size() > 1) {
            // We're only allowed one.
            throw args.createError("You're only allowed one of those!");
        }
    })
    .build();

To top this off, note we already have an "only one" modifier, so let's use that!

Parameter.builder().key(Text.of("test"))
    .valueParameter(CataloguedValueParameter.STRING) // or .string()
    .onComplete(() -> Lists.newArrayList("test1", "test2")) // No need to alter this!
    .usage((text, source) -> {
        // At this point, the value has not been parsed, and an arg from TokenizedArgs has not been picked.
        if (source instanceof ConsoleSource) {
            return Text.EMPTY;
        }
        return Text.of("[", text, "]");
    })
    .modifiers((key, source, args, context, parsingContext) -> {
        // At this point, the value has not been parsed, and an arg from TokenizedArgs has not been picked.
        if (source instanceof ConsoleSource) {
            return; // This just does not parse the element
        }

        parsingContext.next(); // Get the value
    }, CatalogedValueParameterModifier.ONLY_ONE) // or just add .onlyOne()
    .build();

Now, these modifiers are chained, and will execute one after the other!

@dualspiral

Big undertaking, I'll be keeping a close eye on this.

To speak on the whole API impl that is happening currently, it was never meant for the API to hold any real implementation and this was always viewed as a mistake. As time keeps marching on, more and more impl in the API will be moved out and into SpongeCommon. As for having a shaded lib for the command impl, while that is a nice perk I don't see it being a priority and would honestly leave it up to community members to step up and do so as it doesn't benefit our priority which is SpongeCommon.

I believe the majority, if not all, of the implemention should be within SpongeCommon as it gives the flexibility to implemention writers to build a system that meets their exact needs and that has been the direction of everything overall in Sponge as that was its goal. Due to Sponge's code being licensed under the MIT license, it's trivial for them to utilize Sponge's implemention code if they so desire.

I don't really see the merit in a library either, due to them already being able to include the code they need, and there currently only being a few active implemention of the API.

The purist in me says that it should be this way, which is the direction I'm going in now, and I'm quite happy not to change course on that!

it was never meant for the API to hold any real implementation and this was always viewed as a mistake.

I would actually take the view that it is rather handy for the API to contain some implementation. Having gone about implementing CanaryLib - I was always very thankful that large parts of it were already implemented, and required myself very little effort to get working.

@jamierocks

Then you would consult the most complete implementation and coordinate parts of it being separated into libraries. The goal of the API is provide the specification and not dictate how it is implemented. It says "This is what it should be" not "Here is step 1, 2, 3...etc to implement it."

I think you need to drop the system of backwards compatibility with this pr - let this go into a new major version, and make all the changes that should be made.

I'm honestly happy to do that and take up the previous names of classes, but my worry about doing so is that we give developers little chance to change between versions - though the solution to that would be to merge this at the start of a new major API development cycle.

I'm quite happy for the discussion to happen here about that, and I'll be seeking opinions from the Sponge leads about how much we break backwards compat closer to the time, should this all work well. Right now, I'm more concerned about bolting together my logic and making sure what I'm proposing will work, I can always rename and remove classes later.

I'd much rather this just break compatibility.

I think maintaining compatibility limits what you can accomplish and makes this more challenging to implement. Sponge has always been willing to make beneficial changes even if they're breaking, and I think this is such a large change that maintaining compatibility will probably just see people waiting until the last minute.

I would be fine with the old solution being completely gone in order to make a new, great system that plugin developers will be happy to switch to, even if it means maintaining two separate versions for a little while.

Does this handle Selectors better then the current system?

@ryantheleach As it stands, no. What specifically needs to be resolved with that? Is there an issue?

I'm not saying it should be necessarily addressed, the cure might be worse then the disease.

But as it stands with Sponge offered commands and selectors, is that unless the command supports

1. Processing more then one entity,
2. Having a command element that's aware of, and can parse selectors.
3. and that the plugin bothers to actually loop, rather then taking the first element of the collection.

then selectors don't end up working.

Ideally, selectors that pass more then one entity, to a command element that only takes one entity, would cause the command to be run for each entity.

Not sure what should happen for elements that take 2 arguments that take 1 entity each however..

Processing more then one entity

In theory, selectors should still work, if only one entity can be processed. Will have to make sure that happens.

Having a command element that's aware of, and can parse selectors.

Right now, I have https://github.com/SpongePowered/SpongeAPI/pull/1587/files#diff-607ad657484bde707fee0943937768a1R44 exposed in the API, because I think that it's useful for other plugins to be able to extend it. HOWEVER, what I think might be better is if we provide a modifier that attempts to parse a selector if it detects on. That way, we could add a supportsSelectors(<selector options of some sort>), which then filters on the entities that should be returned (so, if only players, etc.)

and that the plugin bothers to actually loop, rather then taking the first element of the collection.

Maybe by making selector support explicit, maybe it could give an indicator to the developer that more than one entity might be returned. This points to a more general problem though, in that many plugin developers don't realise that many standard parameters can return more than one object for one entry - the user and player parameters come to mind. That's what onlyOne is used for. It needs to be more explicit as to what might happen.

Ideally, selectors that pass more then one entity, to a command element that only takes one entity, would cause the command to be run for each entity.

I see the logic in what you're saying, but I think that's going to confuse things - some of my selector accepting commands do take account of multiple entities and this would break that. I think that's better left out.

Seems like if this succeeds, then #1605 will become useless. But the things there would definitely benefit from being included.

Also, something with tab-completion:

List<String> validCompletions = ...;
if (args.hasNext()) {
    String next = args.next();
    validCompletions = validCompletions.stream().filter(str -> str.startsWith(next)).collect(Collectors.toList());
}

This gets sort of annoying. Maybe have ValueCompleter.culling(Supplier<List<String>>) to automatically provide that behavior?

Thought that came to me through private conversation, like a boolean parameter, would there be sufficient interest in a Tristate parameter? I know GP uses one, but I wonder if there might be other interest in it.

It would basically be sugar for an Optional<Boolean>, no?

https://jd.spongepowered.org/6.0.0/org/spongepowered/api/util/Tristate.html

Beware, long comment. This is designed to talk about the current state of play, and I am possibly going to post this to the forums soon to try to get maximum visibility and feedback from the community, including those that don't use Github.

Introducing:

The new Commands API - A Primer

This explanation is designed for developers who use commands in their plugins - which is probably all of you. For Sponge API 8, we're undertaking one of the biggest refactors to the API - overhauling commands.

The reasoning behind it can be found in the associated PR, so we'll concentrate on how a command will work after the API changes.

Cause and Context

Sponge as an API and a system is now very much centred around the tracking of causes. Events have a cause, and now commands do too. The source of a command will now ultimately be the Cause of the command. Our cause tracking is already best in class, and with very little effort we can now make use of that in the Command system. Thus, most of our methods will now include a Cause parameter.

This fits in well with Minecraft 1.13, as the cause of a command is no longer directly a CommandSource, there will be the notion of a source and a target, the target assuming the role of the traditional CommandSource. Usually, this will be the owner or creator of the nominal source of the cause, so will be the first CommandSource in the cause stack - but not always. There are a few edge cases, however, where we might need to execute the command as someone, even if they are not responsible for the command, sudo like, if you will.

As Zidane puts it:

I'm of the firm belief that once the concept of "Thing A runs command but executor is to see Thing B as invoker" comes about, singular "CommandSource" makes no sense anymore. Its the entire reason why the event API went to the Cause system: one or more things triggered the event.

This is one of the discussions around the API, do we do away with the CommandSource completely, or do we still have a nominated source/target?

At the basic level (as it is at the moment), the process method on a command is now:

CommandResult process(Cause cause, String arguments) throws CommandException;

There are two directions we can go with this to define the target of this. Either:

• Update the method parameter to include a CommandSource, indicating the target of the command, who all the permission checks will be done under
• Make more use of the Contexts in the Cause object to define how a command should respond

My current implementation (which is this way to test things and is in no way complete) is to add three keys to the context that can (but don't have to be) used (there are four, but the fourth is likely to be removed):

• COMMAND_SOURCE (likely to be renamed as COMMAND_TARGET or something like that if this goes ahead): this contains what the traditional CommandSource would be. If not present, would be the Cause#first(CommandSource.class).
• COMMAND_PERMISSION_SUBJECT: the Subject permission checks should be done on. If not present, defaults what the COMMAND_SOURCE would be.
• COMMAND_LOCATION: the location that the command might be centred about (but otherwise not attached to an entity)

My thought is that with the combination of the Cause and Context, we can get a lot of information about the entire context of a command execution.

There are more discussions to be had around the form of the command methods and how they should work. They will be discussed in later sections.

Starting at a low level - the Command interface

Some of you will use the ComandCallable interface to manage your commands - this has now become the Command interface. All of the methods that were on the original interface are now on this interface, except they all now take a Cause instead of a CommandSource.

One thing that we need to decide is whether the command target is a parameter on these methods, rather than expecting a developer to get the associated keys and checking for their presence first. So, the question is do we have:

CommandResult process(Cause cause, String arguments);

boolean testPermission(Cause cause);

or:

CommandResult process(Cause cause, CommandTarget target, String arguments);

boolean testPermission(Cause cause, Subject subject);

My feeling that we'll likely go for the latter, as it provides a greater hint as to what the standard course of action is. In both cases, developers can solely rely on the Cause if they prefer.

Going higher level - the Command.Builder

The CommandSpec.Builder of API 8, Command.Builder is your new one stop shop for building a command. There are a few key differences that developers must be aware of:

• "arguments" are now "parameters", as this describes what they actually are.
• Some builder methods will start with set. This indicates that such an action will replace anything set previously - setExecutor(CommandExecutor) called twice will end up with only the second executor being set.
• Builder methods without set as a prefix will build on the previous state. Calling parameter(Parameter) twice will add the result together - in this case, resulting in two parameters in the command.
• Pluralised varargs methods are the same as chaining the singular varient together - parameters(Parameter1, Parameter2) is the same as parameter(Parameter1).parameter(Parameter2)

The CommandExecutor and TargetedCommandExecutors

The CommandExecutor is currently one method:

CommandResult execute(Cause cause, CommandContext context) throws CommandException;

This is subject to the previous discussion on whether the target of a command should be included in the function definition or not. However, something I noticed a lot of developers (myself included) did was build a command that is only suitable for, say, a player or the console. So, I've added the ability to add a TargetedCommandExecutor<T extends CommandSource>, which you can use from the builder using targetedExecutor(TargetedCommandExecutor<T> executor, Class<T> sourceType). This is also a one method class (where T extends CommandSource):

CommandResult execute(Cause cause, T source, CommandContext context) throws CommandException;

The idea is that you can then add multiple targeted executors to separate logic for console and players, for example. You can also add a custom error message for any targets that don't fit your requirements on the builder by calling setTargetedExecutorErrorMessage(Text targetedExecutorError);

This addition should save some boilerplate code where you have to write:

if (!(src instanceof Player)) {
  throw new CommandException(Text.of("You must be a player to run this!"));
}

// Command code

Child commands

We are retaining the ability to add subcommands using the child() methods, they are relatively unchanged. However, there are some new methods pertaining to child commands that should fix some longstanding difficulties, from both a UX side and a developer side.

• setRequirePermissionForChildren(boolean required);: previously, subcommands required the parent command's permission to be accessed. Now, subcommands do not need to tied to the parent permission - but by default, this is enabled. This does not affect any checks the child does itself.
• setChildExceptionBehavior(ChildExceptionBehavior exceptionBehavior);: One of the biggest problems that players have with subcommands is that if the subcommand fails, and then the parent fails, the error message and subsequent usage you get is for the root command. This allows you to customise how this would actually work. There are three behaviours:
  • RETHROW: if a subcommand fails, then do not fall back to the command executor - display the error for the subcommand. This will be the new default.
  • STORE: if a subcommand fails, store the exception, but try to execute the parent command. If the parent fails, pass this error to the method along with the parent's error, display both.
  • SUPPRESS: Current behaviour, if a subcommand fails, discard the error and fall back to the parent.

We expect most developers to use RETHROW or STORE as they see fit. For those of you that use Nucleus, an example of what STORE might look like is if a subcommand errors in Nucleus - though it is unlikely to be exactly that formatting.

Flags

While this was previously part of GenericArguments, I took the decision to split this out from the parameter system as there was a lot of confusion as to how the actual system worked - there has been at least one instance where someone thought that you needed a flag parameter per flag - when you should only have one flag parameter. It made enough sense to set it separately.

There is a new class, Flags, and an associated builder, Flags.Builder. The javadocs should be fairly self explanatory, but there are a couple of things to note:

• Specifying a long flag no longer requires a - at the beginning of a string. So, before, abc specified three flags, -a -b -c. Now, it specifies the long flag --abc.
• UnknownFlagBehaviors specify what happens when a flag like parameter is encountered, but not recognised. The default behaviour is, as before, ERROR.

Parameters

The most drastic change to the API is parameters - previously known as arguments/command elements. The GenericArguments class contained a lot of parsers for commands to use, but is not suited to API/impl splitting, and doesn't fit with the rest of the Sponge ecosystem where builders prevail. It turns out that parameters are actually well suited to the builder pattern.

Replacements for GenericArguments.seq and GenericArguments.firstOf

Before I describe the parameter ecosystem, it's worth mentioning that the replacement for these are Parameter.seq(...) and Parameter.firstOf(...). Of course, these return builders too, so you can have that fluid style you always wanted!

Parameter.firstOf(first).or(second).or(third).build();
Parameter.seq(first).then(second).then(third).build();

This will allow you to choose your style, some of you may like having the builder pattern for this. We give you that choice.

The Parameter class

Previously, when defining your own custom parameter, you extended the CustomElement class. This was a one size fits all class that was used for both parameter parsing and modifying parameters. The closest analogue to this class is the Parameter interface. Parsing methods take the parameter key, a CommandArgs (like before) and a CommandContext (like before, but expanded). A Parameter generally takes one (or more) arguments from CommandArgs, turns the string into a usable value (or throws an exception) and inserts them into the CommandContext under the provided key.

However like Command, we have a Builder that can be used to piece together your element, which we strongly recommend you use.

The Parameter.Builder

Parameter.Builder is our high level solution to parameters. Such a solution allows us to separate the logic of parsing an argument (e.g. methods that took a text key in API 2+ - GenericArguments.bool(key)) and any modifiers (e.g. methods that took another CommandElement - GenericArguemts.optional(CommandElement)). However, we provide more power than that by breaking down the different operations into different classes.

Using standard elements

A parameter created though Parameter.Builder consists of three main elements:

• A text or string key - setKey(String/Text)
• One ValueParameter - setParser(ValueParameter)
• Zero or more ValueParameterModifiers - modifier(ValueParameterModifier)

Most Sponge standard parameters and modifiers have shortcuts on the Parameter and Parameter.Builder classes, however, in order to make this easier. So, to define an integer:

Parameter.integerNumber().setKey("int").build();

Or, to define a parameter with two strings, but requires the permission.strings permissions:

Parameter.string().setKey("strings").setRequiredPermission("permission.strings").repeated(2).build();

Or, an optional Player, but also make sure there is only one if one exists:

Parameter.player().setKey("player").optionalWeak().onlyOne().build();

Or one of "eggs", "bacon" or "spam", defaulting to "spam" if nothing is chosen:

Parameter.choices("eggs", "bacon", "spam").setKey("choice").optionalOrDefault("spam").onlyOne().build();

You can also use the setParser(ValueParameter) and modifiers(ValueParameterModifier) functions directly should you want to. The last two examples could also be written as:

Parameter.builder()
    .setParser(CatalogedValueParameters.PLAYER)
    .setKey("player")
    .modifier(CatalogedValueParameterModifiers.OPTIONAL_WEAK)
    .modifier(CatalogedValueParameterModifiers.ONLY_ONE)
    .build();

Parameter.builder()
    .setParser(
        VariableValueParameters.staticChoicesBuilder()
            .choice("eggs").choice("bacon").choice("spam").build())
    .setKey("choice")
    .modifier(
        VariableValueParameterModifiers.defaultValueModifierBuilder()
            .setDefaultValueFunction(cause -> "spam").build())
    .modifier(CatalogedValueParameterModifiers.ONLY_ONE)
    .build();

You may also change the returned suggestions and usage text using setSuggestions and setUsage, to make your own parameter without having to create an entire new ValueParameter.

You may have noticed that the command key has been separated from the ValueParameter. This is so that we can use the same object for multiple parameters, hopefully reducing the footprint of your plugin.

Modifiers

It's worth talking about how modifiers work. For the most part, when parsing an element, they work like they did in API 2-7 - they wrap around the parser. So if we called modifier(m1).modifier(m2), m1 is called first, which calls into m2, then the parser. Control then returns back through the chain, through the remainder of m2 then m1. Therefore, order is important with modifiers, though one of the benefits of moving implementation to implementation is that we can make exceptions if we need to - one might be optional, moving that to the beginning of the chain, so its logic can execute after the parser has parsed and other modifiers have had their say.

The javadocs of ValueParameterModifer tries to explain this a little better.

Defining custom argument parsers

Defining how to parse an argument (e.g., parse and integer) takes the form of the ValueParameter, which consists of the following three functional interfaces:

• ValueParsers take arguments from a CommandArgs and returns a value (basically, parseValue in CommandElement). This can be used in setParser of the parameter builder.
• ValueCompleters provide tab complete suggestions for a parameter (complete). This can be used in setSuggestions of the parameter builder.
• ValueUsages define what text is sent back, if any, for the parameter when usage is requested (getUsage). This can be used in setUsage of the parameter builder.

So, if you wished, you could create a custom parameter in a Parameter.Builder like so:

Parameter.builder().setKey("key")
    // value parsers return an `Optional`.
    .setParser((cause, commandArgs, commandContext) -> Optional.of(commandArgs.next())) 
    // value completers return a `List<String>`.
    .setSuggestions(
        (cause, commandArgs, commandContext) -> 
            Sponge.getServer().getOnlinePlayers().stream().map(x -> x.getName()).collect(Collectors.toList())
    // key is the key set in `setKey` as a `Text`.
    .setUsage((key, cause) -> Text.of("String ", key))
    .build();

You can add any modifiers that you wish to this, though it's worth noting that modifiers won't override anything you add to setSuggestions or setUsage. You can use setSuggestions and setUsage to override those specific parts of any ValueParameter you set in setParser, so the above string -> string parser could be written as:

Parameter.builder().setKey("key")
    .setParser(CataloguedValueParamters.STRING) 
    // value completers return a `List<String>`.
    .setSuggestions(
        (cause, commandArgs, commandContext) -> 
            Sponge.getServer().getOnlinePlayers().stream().map(x -> x.getName()).collect(Collectors.toList())
    // key is the key set in `setKey` as a `Text`.
    .setUsage((key, cause) -> Text.of("String ", key))
    .build();

or even:

Parameter.string().setKey("key")
    // value completers return a `List<String>`.
    .setSuggestions(
        (cause, commandArgs, commandContext) -> 
            Sponge.getServer().getOnlinePlayers().stream().map(x -> x.getName()).collect(Collectors.toList())
    // key is the key set in `setKey` as a `Text`.
    .setUsage((key, cause) -> Text.of("String ", key))
    .build();

Defining custom argument modifiers

A modifier is an element that can manipulate the parsing of an element before or after the actual parsing, suggestion grabbing or usage text display takes place. Modifiers implement that ValueParameterModifer class.

Assume that we have two modifiers, called though the builder using modifier(m1).modifier(m2). Modifier m1 will get called first:

The main method in the modifier is the onParse method.

void onParse(Text key, Cause cause, CommandArgs args, CommandContext context, ParsingContext parsingContext)
            throws ArgumentParseException;

This method is called before the associated ValueParser is called. This method is designed to wrap around to call to the parser - this method should pass control to another modifier using ParsingContext#next(). So, the body of onParse can be thought of like this:

void onParse(Text key, Cause cause, CommandArgs args, CommandContext context, ParsingContext parsingContext)
            throws ArgumentParseException) {
    // do logic before parsing
    if (someErrorCondition) {
        throw args.createError(Text.of(error)); // prevent parsing from continuing, command should not execute.
    }

    // Go to the next modifier or parser in the chain
    parsingContext.next();

    // Do any checks. Do not assume that anything has been parsed, another modifier may have prevented it.
    if (someOtherCondition) {
        throw args.createError(Text.of(error2)); // prevent parsing from continuing, command should not execute.
    }

    // No error, let previous handler finish up
}

ValueParameterModifiers also have two other methods:

List<String> complete(Cause cause, CommandArgs args, CommandContext context, List<String> currentCompletions)
Text getUsage(Text key, Cause cause, Text currentUsage)

Modifiers will simply be called after the suggestion and usage is determined by the ValueCompleter/ValueUsage. The modifiers will simply be called in order.

Catalogued parameters and modifiers

If you create a parameter or modifier and wish to make it accessibile to other plugins, you can register it in the Sponge registry. Simply extend the CatalogedValueParameter or CatalogedValueParameterModifier interfaces instead of the standard interfaces. Other plugins can then get your parameter from the Sponge registry, should they wish to.

Command Events

Currently, Sponge has the SendCommandEvent. I propose to deprecate this and change this into three events, all subinterfaces of the new CommandExecutionEvent:

• Pre: the SendCommandEvent as it is today.
• Selected: when a Command has been selected for processing, the CommandMapping is available for inspection, the command can no longer be changed (but can be cancelled)
• Post: when a Command has completed execution

Dispatcher: Subcommand Discovery

It's been noted that it's not easy to find out what subcommands that a Command built using Command.Builder might have. I've introduced a getCommandNode on Dispatchers (and therefore, the CommandManager), which allows you to get the subcommands and relating mappings by walking a command tree through the use of CommandNodes (name subject to change).

Next Steps

Our next steps is to get this tested and gather feedback - it's almost ready, though there are some bugs that squishing first. We hope that you like these changes, while we accept that it'll be some work for some of you to bring your plugins inline, this will allow us to more easily make implementation changes and will allow for a stable, more featureful API for years to come. This includes being able to keep up with Brigadier, Minecraft 1.13's new command library that will sync up usage for each player.

So I touched on this in the livestream a bit, but I'll go into more detail here. So, I think a lot of us that use the current command system is guilty of is not using onlyOne in the places it should be used. I definitively know I am.

The most common case of that is taking a player in a command. Usually, I just need a single player, but I forget to use onlyOne. For normal stuff, it works fine, but if someone tried to use a selector for example, then I have no idea what would happen.

I'd love to see some sort safeguard or maybe even a simple warning that is printed when you only get a single value for some value that can have multiple values (it probably won't always have multiple values, but would still be nice for the safeguard to kick in

IMO a goal of any command API should be that you could rewrite all the vanilla commands in it. So the ability to use Brigadier's new NBT stuff should definitely be in it.

I've had a quick look at this.

Do you think you could encode into the generics of Parameters whether they return 1, return many, or return maybe 1?

I predict someone's next question would be, how would you get that information out of the context?

well if we reused the parameters as the command keys rather then texts, it would give you the typing information needed.

e.g. Parameter<Optional<Player>> target;

Optional<Player> targetPlayer = context.getOptional(target);

Or am I crazy?

I just want to get rid of this pattern.

context.getArgument(Text.of("player")).get()//throws exception that makes sense to a java programmer, not a user. gets wrapped by commandexception somewhere, and presented to end user as untranslateable error message.

In theory, anything is possible. In practice, not really. All you are doing is moving the problem around. You are moving the generic check from the executor to the parameter and changing the key to be the parameter itself.

Problems that might occur are:

• Parameters, and CommandElements before them, are usually specified inline in the command(spec) builder. This makes parameter creation more cumbersome.
• What if someone creates two parameters with the same key with different types returned? You're still going to get a ClassCastException if you don't think about it properly.
• If you're returning an Optional and you call get(), you're still going to get a user meaningless exception.

There are two ways around this, of which, I've gone for the second:

• Create a ParameterKey<T> that contains a type and a Text object with the name of the key and use that. Unfortunately, I can see developers not storing the keys and recreating them each time - while I'm not against additional steps I fail to see it resolving the issue completely.
• Add CommandContext#getOneUnchecked(String key), that way, I can customize the message if the element doesn't exist. I haven't necessarily given this a great deal of thought, but I could do:

T getOneUnchecked(String key) {
    try {
        T ret = (T) map.get(String key);
        if (ret == null) {
            throw new CommandException(...);
        }
    } catch (ClassCastException ex) {
        throw new CommandException(...);
    }
}

If the returned object is an optional (so using CommandContext#getOne(String key)), we could probably put it through the same check, just with wrapping in an optional instead.

I admit that none of this is ideal at all, but using the parameter as a key is going to complicate things for some developers I think - best we protect from some mistakes as gracefully as possible rather than adding a barrier that budding developers might not understand. However, we should also impress upon the developers that they need to test their code. We cannot protect against everything, because if we do, the system will become unusable.

What if someone creates two parameters with the same key with different types returned?

In my mental model I didn't consider this as a possibility. The way I've imagined commands is one parameter is associated with one key, with one type, it's just a matter of whether that type is a union type or some other wrapper.

I can understand wanting duplicate parameters, but they are more like parameter archetypes, they still need to be built with a key anyway, which then uniquify'd them in my head.

"re usually specified inline in the command(spec) builder" traditionally yes, but you either needed to supply a key (which in my plugins I've always used a field constant to reduce errors) so I would have been using a field constant for the parameter instead.

If you're returning an Optional and you call get(), you're still going to get a user meaningless exception.

You wouldn't be returning an optional, unless it was an optional parameter, in which case it's on the user.

Create a ParameterKey that contains a type and a Text object with the name of the key and use that. Unfortunately, I can see developers not storing the keys and recreating them each time - while I'm not against additional steps I fail to see it resolving the issue completely.

This is the path that I would have taken, but my coding style is probably different, I was confusing the ideas of ParameterKeys and Parameters I think.

Add CommandContext#getOneUnchecked(String key), that way, I can customize the message if the element doesn't exist. I haven't necessarily given this a great deal of thought, but I could do:

This works, and would be better then the current situation, without changing what people are used to too much.

I admit that none of this is ideal at all, but using the parameter as a key is going to complicate things for some developers I think - best we protect from some mistakes as gracefully as possible rather than adding a barrier that budding developers might not understand. However, we should also impress upon the developers that they need to test their code. We cannot protect against everything, because if we do, the system will become unusable.

Sure. I just thought this was one area that we could improve upon, but it looks like I hadn't considered all angles.

What's the status of this?

I think we should consider re-opening this PR to have a fresh list of comments and clearer description with the updated status. This PR is becoming too much for GitHub to load sometimes.

The PR has mostly stalled due to my current work and life workload (reasons for which I'll restrict to private circles), as well as the stable-7 cleanup, but it does work (mostly) and probably doesn't need much more on it as a first pass, though I do have a couple of things I want to do

However, I'm also considering breaking this into two:

• Move the low level stuff, such as CommandResult -> Interface and CommandCallable -> Command into a separate PR with a view to getting it merged quickly, such that the current high level API can sit on top of it, so, barring a few renames, will be more of a binary break than an API break.
• High level stuff, that is the Command.Builder can then go into a separate PR and possibly an API 7 plugin for testing.
• Possibly consider a third for events, etc.

There is a lot in this PR, more than I anticipated, and it might allow things to go more quickly if it's broken up as such.

As this targets bleeding, I also don't mind getting stuff in that works, then tweaking it after, such as where to include the Cause in the interfaces etc.

I'll try to spend a bit of time on it later this week when I have a bit more time, but it may be a couple of weeks before I can really get onto this with anger.

This PR looked so promising and yet it have stalled, any updates?

Closed in favour of #1958