EngineHub / Intake

IoC-oriented Java command parsing library
GNU Lesser General Public License v3.0
98 stars 21 forks source link

Implement completion suggestions provided by argument providers. #23

Closed filipejsfreitas closed 5 years ago

filipejsfreitas commented 8 years ago

This commit implements argument-level command completion suggestions by using the providers for the given arguments, meaning that no extra special suggestion methods on the command classes are necessary.

Note: This commit breaks any current implementation of the Provider class, because the method signature for the getSuggestions method has changed (one argument was added), but that shouldn't be too big of a deal. Some refactoring and you should be up and running again.

Note 2: This is not a tidy solution. Instead of storing parameters as a list, in the ArgumentParser class, I had to change it to a LinkedHashMap to not only be able to get the user provided arguments and their ParameterEntry objects directly (for performance reasons), but also because we need to store the order in which elements are added to that map. If someone has a better alternative, be my guest!

Note 3: The actual code to make this work is very small, most of the changes are just to "unbreak" the code because of the method signature change I talked about earlier.

Note 4: Code compiles and has been tested.

Edit: You might ask, why another PR to implement argument completions? Well, because all other PRs I've seen were using custom completers on the Command classes. That's pretty useless considering providers already have a system for dealing with this. So, what I've done is, instead of creating a whole array of new classes to achieve this goal, I've just taken the existing system and expanded it to support suggestions properly. This should make it really easy to implement suggestions in existing projects you might have, plus, prevents code duplication and promotes organization by placing code about the same thing in the same class.

TheE commented 8 years ago

I too prefer this solution over other PRs. However, there is a mismatch between argument and suggestion parsing (which is somewhat caused by the original implementation):

Modifier annotations are not accessible when parsing suggestions, therefore suggestions cannot take individual modifiers into account.

As an example, a command takes a planet object that is parsed from string input, but only accepts planets that have at least one moon:

@Command(aliases = "removemoons", desc = "Removes all moons from a planet")
public void removeMoons(@HasMoon Planet planet) {
   planet.removeMoons();
}

The parser can check for the @HasMoon annotation and query only planets that have at least one moon. When parsing suggestions this is not possible, there is no way to know way to know which valuator annotations a certain command has in place. One could only suggest all possible planets, including those that do not match a command's validators.

filipejsfreitas commented 8 years ago

Yes, I agree with you about that mismatch, however, like you've said, it's caused by the original implementation and not really me. I remember trying to get CommandArgs to be passed in, but for some reason, I don't quite remember what reason though, I couldn't get it to work without too many changes. However, it is a good suggestion to throw in the Annotations.

The intent behind the getSuggestions method accepting the Namespace as an argument was for a similar reason: CommandSender. I would like to send suggestions that are specific to the command sender. And, with the Namespace, it would also be possible to throw in other stuff into the namespace before it reaches the providers.

Your suggestion fits my line of reasoning perfectly, so, I will try to get another commit to implement this change, if at all possible (it should be, but I don't quite remember right now).

Edit: Will get another branch with those changes first, and maybe later will submit them to the master branch, but I need to make sure stuff will work fine before I do so.

filipejsfreitas commented 8 years ago

Actually, after looking at the code, this change is really easy to implement (it's literally just changing the method signature and passing in the argument, which is easy to get). Doing it now.

filipejsfreitas commented 8 years ago

Alright, your suggestion has been implemented. In the process, however, I noticed that Jenkins was throwing out errors in the unit tests but still passing the build and creating the artifacts. Now that's weird. Anyway, it's all fixed up now.

AustinLMayes commented 5 years ago

RIP