Commands refactor

[zml2008]

This PR pretty much entirely replaces the commands system.

The new system is a bit more structured -- it contains facilities for argument parsing.

An example of the version of this system included in PEX is at https://github.com/PEXPlugins/PermissionsEx/blob/master/src/main/java/ninja/leaping/permissionsex/command/ParentCommands.java#L37
(there are a few differences in this version, mostly relating to how command names are added, the fact that Text is used for stuff rather than PEX's abstracted wrappers, and to add some more flexibility).

I have included #552 on top of my changes since a lot of command argument stuff has changed, to help @boformer in updating their PR.

Example usage

This registers a simple command. It would be put in the plugin's PreInitializationEvent listener.

        this.game.getCommandDispatcher().register(this, CommandSpec.builder()
                .setArguments(seq(string(t("item")), optional(integer(t("count")), 1)))
                .setExecutor(new CommandExecutor() {
                    @Override
                    public CommandResult execute(CommandSource src, CommandContext args) throws CommandException {
                        final Optional<String> item = args.getOne("item");
                        final Optional<Integer> count = args.getOne("count");
                        src.sendMessage(Texts.of("You've gotten ", count.get(), " ", item.get(), count.get() != 1 ? "s" : ""));
                        return CommandResult.builder().affectedItems(count.get()).build();
                    }
                })
                .build(), "testcmd");

Want to work with all the provided arguments as a list?

        this.game.getCommandDispatcher().register(this, CommandSpec.builder()
                .setArguments(allOf(string(t("args"))))
                .setExecutor(new CommandExecutor() {
                    @Override
                    public CommandResult execute(CommandSource src, CommandContext args) throws CommandException {
                        final Collection<String> argList = args.getAll("args");
                        src.sendMessage(Texts.of("Arguments provided", argList.toString()));
                        return CommandResult.empty();
                    }
                }).build(), "rawargscmd");

Major Components

CommandSpec: Immutable object that contains the execution data for the command, including permission, expected arguments, executor, description, and extended description.

Dispatcher: Handles the logic of registering, calling, and tabcompleting commands (the logic to CommandSpec's data)

CommandExecutor: Single-method interface that takes the sender and parsed arguments and performs the command's action

CommandElement: The parent class for any type of command argument. These are often composed to create a complex argument chain. CommandElements are defined in GenericArguments and GameArguments (in o.s.a.util.commands and o.s.a.service.command respectively)

[AlphaModder]

@zml2008 zml, I'm trying to implement changes from #539 into this, but I'm having trouble understanding some parts. Is there any time on IRC or other places I could contact you?

[stephan-gh]

Is this really necessary? This is an extremely bad method name and you could get almost an as short name if you just add an static import for Texts.of()

[zml2008]

it's the traditional name used for gettext

[stephan-gh]

See below:

Using a single underscore character as an identifier for method names is not only making code hard to read if you don't know what this special _ method is doing but it is also discouraged and generates a compiler warning in Java 8: https://travis-ci.org/SpongePowered/SpongeAPI/jobs/58021029#L202-L294

[zml2008]

they can't exactly make a valid method name invalid -- that'd break existing source... but I guess I can use t() or something

[ST-DDT]

use Texts.of() directly with static imports this is only one character more or add String constructors to your exceptions that do the conversion themselfes.

it's the traditional name used for gettext

In the API we have chosen a different one.

[boformer]

I agree with @ST-DDT

[zml2008]

... you do not understand translations

[ST-DDT]

This class looks like a nice utility class thar should be public so everyone can use it.

[ST-DDT]

This way we dont need it 3 or more times (for every package). And we can improve maintainability.

[zml2008]

it's not designed to exist long-term -- it's a placeholder until a proper translation system is present in Sponge

[ST-DDT]

This PR adds some nice features, but IMO this forces commands too much in a specific design/layout.

How can i achieve the tab completion with your suggested system.
Example:
ArenaPvP-options: allowFriendlyFire...
ArenaPvE-options difficultyPerRound....
/myarenas setoptions <Arena> <Option> <Value...>
TabComplete for Option is based on Arena selection
TabComplete for Value is based on Option selection

[nikosgram]

I believe, the best method to register and execute a command is the @sk89q's command-framework from WorldEdit.

https://github.com/sk89q/WorldEdit/tree/master/worldedit-core/src/main/java/com/sk89q/minecraft/util/commands

[ST-DDT]

@nikosgram13 The issue with that system is that annotations do not support Texts -> no color + hover/click/localisation support.

And i think everyone prefers a different style, thats why i like the current/old system, because you can build everything on top of it. The proposed system is too much limited.

[zml2008]

@ST-DDT tab completion gives each successive argument values from the previous arguments. so you can get stuff from the CommandContext if necessary.

I kinda don't want to expose the CommandSource in the CommandElement -- could run into issues with people doing too much with command sources. All the methods in CommandElement are supposed to have no side effects and be repeatable, meaning anything people do with CommandSources must not change game state at all.

I do see how adding the commandsource could be easier though, so I dunno

[boformer]

This should be an interface/not final (like CommandCallable).

Some plugins may want to implement a custom checkPermission or getDescription method.

[boformer]

Please add a detailed PR description. I don't understand how this system works.

Right now I think that the system we currently have is better than this one. Please don't merge it in its current state!

Argument Parsing is a nice feature, but I think that it should be optional. There should still be a way to get a simple string array (or a single string) of the arguments.

I don't understand why Sponge needs such a complex command system. It should be minimal and flexible, so that other command systems can be built on top of it. The system proposed here is very static (final classes instead of interfaces). I think it should be a third-party library.

Is there anything that makes it impossible to implement this system on top of the existing command system?

[Zidane]

@boformer

I disagree on the point of providing a String of arguments, there should be standardized parsing in play.

[zml2008]

The point of this PR is for every plugin's commands to be handled similarly -- so that every plugin benefits from the same level of usability and users don't have to learn 384854 arguments systems for each plugin and which plugins support quoted arguments and escapes any flags and whatever.
It is still possible to go around the API and get a raw string (we do it for MC/mod commands) -- it's just not recommended because it increases confusion for users.

[Zidane]

Absolutely agree with @zml2008

[sk89q]

Realistically there will be a lot of people (I included) that will not be providing standardized access to the commands because switching over commands is only going to be needless work that introduces new bugs.

I wouldn't be surprised if the situation was the same as with Bukkit, except that now people have to figure out something complicated to bypass.

Y'know, it's possible to just introduce parsing into the current Sponge command code as a library and that would give you fairly standard argument parsing, without any of this proposal's complexity and with all the flexibility and power of the current system. It may be easy to bypass if it's optional, but it is easy to bypass the entire concept with this proposal too. If you wanted to be able to inspect arguments, then there's no reason why that can't be done and hasn't been done before, which would actually make it worthwhile to use this theoretical bundled argument parsing library.

[AlphaModder]

@nikosgram13 That is what we used to use, before this.

[zml2008]

I see where you're coming from sk -- I'm going to be overriding the commands system in PEX. I've been thinking about making CommandSpec implement CommandCallable -- mostly I've been worried that that doesn't encourage users to do what we want enough.

@AlphaModder we've never used sk's full command API -- what we've used is some super basic stuff that's very low-level. WE uses sk's full commands API, which is imo also a good commands API.

[ST-DDT]

@sk89q I agree with you.

The point of this PR is for every plugin's commands to be handled similarly -- so that every plugin benefits from the same level of usability and users don't have to learn 384854 arguments systems for each plugin and which plugins support quoted arguments and escapes any flags and whatever.

An basic implementation of the CommandCallable would do the same job. And if tutorials mostly deal with that system beginners or writers of small plugins will use it anyway. But if expert programmers like sk89q will not use it and instead they break holes in the system everyone else will follow their example. Because the Command system then as a bad image in general although it might be useful in some places..

I for myself i already know that i will break the suggested system or at least try to apply some wrappers all around it, mainly of these 3 reasons:

• I need multilanguage support
• I want to provide a help to the player if he types something wrong.
  • "Could not find Material STONE_BRIC did you mean STONE_BRICK?"
• I need named command params for some cases.

I would be happy if you would reintroduce the CommandCallable as base interface.

mostly I've been worried that that doesn't encourage users to do what we want enough.

Rules/Limitations alone have rarely encouraged anyone to do what you want. Ask any parents.
You can only remember the authors via the javadocs and by leading by example.

[zml2008]

@ST-DDT except that all those features you talked about are supported in this commands implementation.

I'm not sure where your logic of "if the big plugins stop using the commands API everyone else will too" came from -- even after 5 years of Worldedit, not a lot of plugins use WE's commands API (not like Bukkit offered much anyway).

multilanguage: everything is passed as a Text, which is translatable (I will tweak the Translation interfaces in Text soon to make this simpler)
suggestions for incorrect items: those go in the appropriate CommandElement for the type. In your case, this would be GameArguments.CatalogedTypeCommandElement. (you could even do something like levenschtein difference filtering for tab complete & suggestions)
named command params: you mean like long flags? because those are supported -- see GenericArguments#flags(). And argument lookup is done by name too.

I encourage you to take a look at what the API offers before you start complaining.

[ST-DDT]

I'm not sure where your logic of "if the big plugins stop using the commands API everyone else will too" came from -- even after 5 years of Worldedit, not a lot of plugins use WE's commands API (not like Bukkit offered much anyway).

Because it is not integrated in Bukkit and most tutorials do not use it.
Plugin dependencies are rarely used by beginners.

multilanguage: everything is passed as a Text, which is translatable (I will tweak the Translation interfaces in Text soon to make this simpler)

throw args.createError(t("Unknown long flag %s specified", args));

-> catch(TextException e) throw new TextException(translate(e.getText(), e)

suggestions for incorrect items: those go in the appropriate CommandElement for the type. In your case, this would be GameArguments.CatalogedTypeCommandElement. (you could even do something like levenschtein difference filtering for tab complete & suggestions)

That feature is pretty nice, but if i only support a subset (example helmets), i end up implementing it all myself. I never complained about the parsing part. I don't like being forced to define all parsing stuff when declaring the method. Cherry picking your parsers for some arguments is actually a nice benefit of this PR. EDIT: The ChoicesCommandElement is probably close enough when used with a helper method.

named command params: you mean like long flags? because those are supported -- see GenericArguments#flags(). And argument lookup is done by name too.

Almost, but here is another issue. I want to use the location resolver to not require quotes and --.
Because the number of arguments is implicit
/cmd foo --target="[world] <x> <y> <z>"
if it just could be
/cmd foo target=[world] <x> <y> <z>
I know I could implement my own CommandElement...
But the proposed system does not allow me to use the following at all, even if i want
/cmd foo --target=here
Which is a pretty cool feature.

Wait is there anything i have not created wrappers or own implementations for?
And oh wait, i already have my own parsing from Bukkit in place which is incompatible with the current system.

The parsing part of this PR is probably one of the points i will use it to comply with the expected user experience, but i will not use it in the same context (all parsing first) and with the same responses for every type.

My plugin users don't want to learn my plugin command's structure? Well, don't use it.

PS: I'm also not sure whether Translation supports hardcoded strings, until now i thought the key is used to resolve the string on the client side.

PS2: Please notice i don't have anything (major) against your command system what is not based on personal preference, but i don't like to be forced to do it your way. Especially if it causes a lot of work for me while not adding any new features i couldn't have implemented myself before.

[simon816]

Should this be final?

[Mumfrey]

It doesn't need to be, it has a private ctor.

[ST-DDT]

IMO it should be final in that case.
We did the same for a lot of helper/util classes.

[stephan-gh]

Should be final for consistency with other classes like this one

[stephan-gh]

You can use Objects.hashCode(Object...) from Guava here to simplify this (we use this in a lot of classes already)

[stephan-gh]

Can you also fix these minor formatting problems so we can keep master clean:

service/command/GameArguments.java:242: warning - Tag @link: reference not found: T
util/command/args/GenericArguments.java:643: warning - Tag @link: reference not found: T

util/command/args/CommandFlagsTest.java:28: warning: Using the '.*' form of import should be avoided - org.spongepowered.api.util.command.args.GenericArguments.*.
util/command/args/GenericArgumentsTest.java:29: warning: Using the '.*' form of import should be avoided - org.spongepowered.api.util.command.args.GenericArguments.*.

(Optimizing the imports using your IDE should fix these)

[simon816]

There's a few places where you have double spaces where there shouldn't be:
ArgumentParseException:89
CommandElement:76
CommandFlags:259
GenericArguments:127
GenericArguments:158
GenericArguments:216
GenericArguments:284
GenericArguments:302
GenericArguments:455
GenericArguments:469
GenericArguments:507
GenericArguments:666