-
Notifications
You must be signed in to change notification settings - Fork 30
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
#56 implement autocompletion #112
#56 implement autocompletion #112
Conversation
added new CompleteCommandlet added jline3 and jansi dependencies implemented 1st prompt with autocompletion
# Conflicts: # cli/src/main/java/com/devonfw/tools/ide/commandlet/CommandletManagerImpl.java
added new CommandletRegistry class (implements CommandRegistry) added more features of jline3 to complete commandlet set jline3 version from 3.18.0 to 3.23.0
refactored CommandletRegistry added jline3 autocompletion to Ide (if Ide was run without arguments) fixed multiple initializations warning moved IdeCompleter to new class file added simple completion examples
added jline license added jansi license
cli/src/main/java/com/devonfw/tools/ide/cli/CommandletRegistry.java
Outdated
Show resolved
Hide resolved
String word = commandLine.word(); | ||
List<String> words = commandLine.words(); | ||
// TODO: implement rest of this | ||
Commandlet sub = findSubcommandlet(words, commandLine.wordIndex()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am aware that this is all WIP but just to give some guidance here:
We would need to work on Property
rather than on Commandlet
here to do 2nd, 3rd, ... level completions.
1st level is easy: list all commandlet names/keywords.
Next levels get more tricky:
e.g. 1st = "install"
for 2nd: find this property in InstallCommandlet
as 2nd arg property:
public final ToolProperty tool; |
As it is a
ToolProperty
it can only complete to commandlets that are actually instances of ToolCommandlet
.for 3rd: you would find this property:
public final VersionProperty version; |
And now that we have to complete versions.
The most maintainable approach for implementing all this is making use of polymorphism and delegate the completion to the property itself.
However, therefore you will need to create some completion context containing the IdeContext
together with the information that has already been gathered for the completion (the matching commandlet
and its arguments).
Then a VersionProperty
, ToolProperty
, etc. can implement some method autocomplete(CompletionContext, List<Candidate>)
accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented in a "static" way.
cli/src/main/java/com/devonfw/tools/ide/commandlet/CompleteCommandlet.java
Outdated
Show resolved
Hide resolved
added jline.version property to pom fixed context based help command removed AnsiConsole
removed SystemRegistry and replaced with IdeCompleter added options completion added version completion added tool completion
added tests using tab completion
added version number sorting removed picocli example code
fixed javadoc
fixed version number sorting removed used option and alias removed jansi dependency
implemented first version of option completion with cleanup of used options converted commandletOptions from String to actual Property adjusted tests
added new constant for maximum results to display
adjusted visibility of applyAndRun to private
added setLogLevel method to AbstractIdeContext added loggerFactory class variable to AbstractIdeContext
made sure that all exceptions get catched and reported with an error message
Thanks for your observations. I've adjusted the autocompleter now to catch all exceptions and be able to continue if an internal error occurred. CTRL+C and CTRL+D should work fine too now. |
moved check for invalid patterns to the top to stop autocomplete early before doing other stuff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you maybe split the try block to differentiate between autocompletion and CLI errors?
added another try block for catching run CLI exceptions
Pull Request Test Coverage Report for Build 7462088879
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ready to be reviewed by @hohwille
public CmdDesc commandDescription(List<String> list) { | ||
|
||
List<AttributedString> main = new ArrayList<>(); | ||
Map<String, List<AttributedString>> options = new HashMap<>(); | ||
// String synopsis = | ||
// AttributedString.stripAnsi(spec.usageMessage().sectionMap().get("synopsis").render(cmdhelp).toString()); | ||
// main.add(Options.HelpException.highlightSyntax(synopsis.trim(), Options.HelpException.defaultStyle())); | ||
|
||
AttributedString attributedString = new AttributedString("test"); | ||
main.add(attributedString); | ||
options.put("test", main); | ||
// for (OptionSpec o : spec.options()) { | ||
// String key = Arrays.stream(o.names()).collect(Collectors.joining(" ")); | ||
// List<AttributedString> val = new ArrayList<>(); | ||
// for (String d: o.description()) { | ||
// val.add(new AttributedString(d)); | ||
// } | ||
// if (o.arity().max() > 0) { | ||
// key += "=" + o.paramLabel(); | ||
// } | ||
// options.put(key, val); | ||
// } | ||
// return new CmdDesc(main, ArgDesc.doArgNames(Arrays.asList("")), options); | ||
// TODO: implement this | ||
return new CmdDesc(main, ArgDesc.doArgNames(Arrays.asList("description")), options); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use more descriptive strings than "test" and "description"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jan-vcapgemini Wow. Thanks for this amazing PR. This is really a complex and tough story and you did very well. 👍
Thanks for even adding licenses and the great JUnit tests that are really great to read and follow.
As always I digged in the details and left quite some comments for improvement.
Lets have a call and see how we can complete this story for merge asap.
// Supplier<Path> workDir = context::getCwd; | ||
// set up JLine built-in commands | ||
// TODO: fix BuiltIns or remove | ||
// Builtins builtins = new Builtins(workDir, null, null); | ||
// builtins.rename(Builtins.Command.TTOP, "top"); | ||
// builtins.alias("zle", "widget"); | ||
// builtins.alias("bindkey", "keymap"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where does this come from? Did you copy & paste such code?
According to clean-code avoid committing out-comented code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced with TODO with link to issue.
// TODO: implement TailTipWidgets | ||
// TailTipWidgets widgets = new TailTipWidgets(reader, systemRegistry::commandDescription, 5, | ||
// TailTipWidgets.TipType.COMPLETER); | ||
// widgets.enable(); | ||
// TODO: add own KeyMap | ||
// KeyMap<Binding> keyMap = reader.getKeyMaps().get("main"); | ||
// keyMap.bind(new Reference("tailtip-toggle"), KeyMap.alt("s")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced with TODO with link to issue.
/** | ||
* Initializes jline3 autocompletion. | ||
*/ | ||
private void initializeJlineCompletion() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be wrong but if I read the code correctly this is not "initializing" the "completion" but actually "running it interactively" and also executing what the user finally submitted.
Therefore the method name and the JavaDoc is misleading. I would name the method rather something like runWithInteractiveCompletion
or so...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusted.
try { | ||
initializeJlineCompletion(); | ||
return ProcessResult.SUCCESS; | ||
} catch (RuntimeException e) { | ||
return 1; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
catching the exception here is wrong. The method runOrThrow
intentionally shall not do the execption handling. Further we already have a functional exception handling that considers CliException
that can bring a custom exit code.
Further, I would follow the same pattern as processCliArgument
and return the exit code from the completion method. Maybe here it would increase readability to move the existing behavior to an else
block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a JUnit is calling without arguments (accidentally) - will this make the test hang forever?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusted, but not sure about the returns. Might break/cancel the autocompletion with no way to start it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to re-add a return for the CTRL+D case.
// TODO: add more logic to remove unused keyword | ||
commandlets.add(commandlet.getName()); | ||
commandlets.add(commandlet.getKeyword()); | ||
} | ||
|
||
for (Commandlet commandlet : commandletCollection) { | ||
if (commandlet instanceof ToolCommandlet) { | ||
// TODO: add more logic to remove unused keyword |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better create the next story instead of adding TODOs to main
branch (what we do with the merge as I guess we do not want to wait for perfection with the merge as others already rely on this being merged).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a new issue and linked it in the TODO comment.
toolCommandlets.add(commandlet.getName()); | ||
toolCommandlets.add(commandlet.getKeyword()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have any example where name
can be used via CLI and is different from keyword
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it is pointless to add both name and keyword.
String word = commandLine.word(); | ||
List<String> words = commandLine.words(); | ||
// options | ||
List<Property<?>> optionList = new ArrayList<>(); | ||
for (String singleWord : words) { | ||
if (singleWord.startsWith("-")) { | ||
Property<?> commandletOption = cmd.getOption(singleWord); | ||
if (commandletOption != null) { | ||
optionList.add(commandletOption); | ||
} | ||
} | ||
} | ||
// cleanup options | ||
Set<String> cleanedOptions = new HashSet<>(); | ||
List<Property<?>> properties = cmd.getProperties(); | ||
Set<Property<?>> options = new HashSet<>(properties); | ||
for (Property<?> option : optionList) { | ||
// removes aliases and vice versa too (--trace removes -t too) | ||
options.remove(option); | ||
} | ||
for (Property<?> option : options) { | ||
if (option instanceof FlagProperty) { | ||
cleanedOptions.add(option.getName()); | ||
cleanedOptions.add(option.getAlias()); | ||
} | ||
if (option instanceof LocaleProperty) { | ||
cleanedOptions.add(option.getName()); | ||
} | ||
} | ||
|
||
// adds non options to list | ||
List<String> wordsWithoutOptions = new ArrayList<>(); | ||
for (String singleWord : words) { | ||
if (!singleWord.startsWith("-")) { | ||
wordsWithoutOptions.add(singleWord); | ||
} | ||
} | ||
|
||
if (word.startsWith("-") && wordsWithoutOptions.isEmpty()) { | ||
addCandidates(candidates, cleanedOptions); // adds rest of options without used option | ||
} | ||
|
||
if (wordsWithoutOptions.size() == 1) { | ||
addCandidates(candidates, this.commandlets); // adds all commandlets | ||
} else if (wordsWithoutOptions.size() == 2) { | ||
// 2nd layer.. | ||
|
||
Commandlet commandlet = context.getCommandletManager().getCommandlet(wordsWithoutOptions.get(0)); | ||
if (commandlet != null) { | ||
properties = commandlet.getProperties(); | ||
for (Property<?> property : properties) { | ||
if (property instanceof ToolProperty) { | ||
addCandidates(candidates, this.toolCommandlets); | ||
} | ||
if (commandlet instanceof HelpCommandlet) { // help completion | ||
Set<String> commandletsWithoutHelp; | ||
commandletsWithoutHelp = this.commandlets; | ||
commandletsWithoutHelp.remove(commandlet.getName()); | ||
addCandidates(candidates, commandletsWithoutHelp); | ||
} | ||
} | ||
} | ||
// 3rd layer | ||
} else if (wordsWithoutOptions.size() == 3) { | ||
Commandlet commandlet = context.getCommandletManager().getCommandlet(wordsWithoutOptions.get(0)); | ||
if (commandlet != null) { | ||
properties = commandlet.getProperties(); | ||
for (Property<?> property : properties) { | ||
if (property instanceof VersionProperty) { // add version numbers | ||
Commandlet subCommandlet = context.getCommandletManager().getCommandlet(wordsWithoutOptions.get(1)); | ||
if (subCommandlet != null) { | ||
String toolEdition = context.getVariables().getToolEdition(subCommandlet.getName()); | ||
List<VersionIdentifier> versions = context.getUrls().getSortedVersions(subCommandlet.getName(), | ||
toolEdition); | ||
int sort = 0; | ||
// adds version numbers in sorted order (descending) | ||
for (VersionIdentifier vi : versions) { | ||
sort++; | ||
String versionName = vi.toString(); | ||
candidates.add(new Candidate(versionName, versionName, null, null, null, null, true, sort)); | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems to be a very important core part of this story.
IMHO we should do a meeting to discuss this. So far our CLI is rather simple but common cases are also things like --some-option some-value
where --some-option
is an option property that takes a value (instead of being a boolean flag property). Maybe we can avoid adding this complexity but this code seems to redundantly reimplement the existing CLI parsing but based on JLine3 code. If we have bugs or changes in one place, we will always also have to change the same in the other place.
Further you make parsing code more readable and maintainable if you create a "state object" that wraps the input (e.g. List<String> words
or in other cases Reader
or whatever) together with the current parsing position allowing to consume data (here words
) from that "state object" as a stream of tokens. That was the idea behind CliArgument
or if you want to have a really complex example to look at see e.g. https://github.com/m-m-m/scanner/blob/ecb9a47d1a19796fd26f89dfb984f54477ae96fd/core/src/main/java/io/github/mmm/scanner/CharStreamScanner.java
As discussed earlier my suggestion and expectation would be to delegate the computation of the auto-completion candidates to the property via some new method. You are not utilizing the power of OOP/polymorphism if you do if (property instanceof ToolProperty) { ... } else if (property instanceof ...
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this code is hard to read and needs optimizations.
Was the closest PoC I could get working, looking forward to the meeting.
private void addCandidates(List<Candidate> candidates, Iterable<String> cands, String preFix, String postFix, | ||
boolean complete) { | ||
|
||
for (String s : cands) { | ||
candidates | ||
.add(new Candidate(AttributedString.stripAnsi(preFix + s + postFix), s, null, null, null, null, complete)); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see any invocation using the method directly so why having preFix
and postFix
feature if we never use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was taken from the picocli implementation example could be removed I think.
for (IdeLogLevel level : IdeLogLevel.values()) { | ||
IdeSubLogger logger; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as you copy & pasted this code into setLogLevel
method please replace it here with
setLogLevel(minLogLevel);
FYI: Github suggestion does not work out of reach from the actual code change...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusted now.
renamed initializeJlineCompletion to runWithInteractiveCompletion and adjusted comments
replaced redundant code with setLogLevel(minLogLevel)
adjusted TODO's (added link to issues) removed commented code moved processCliArgument into else block added error codes in case of exceptions changed runWithInteractiveCompletion from void to int to be able to use error codes
replaced Throwable with Exception renamed retrieveCliArgument to initContext
replaced -v and --version with VersionCommandlet fixed CTRL+D missing return and exit code added test for VersionCommandlet autocompletion
changed to more descriptive name and description
removed -v and --version
removed CommandletRegistry
@jan-vcapgemini thanks again for this great PR. You did solid pioneer work to figure out how to make this work. 👍 These PRs copied your work and further improved it. Therefore, this PR can be closed as it is more or less replaced by the other PRs. Credits to you for making this happen - I guess I would have not integrated jline3 myself but now I really love your idea and see it as a extremely valuable enhancement and feature for IDEasy. |
Addresses/Fixes: #56
Implements:
get-version mvn -