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

#56 implement autocompletion #112

Conversation

jan-vcapgemini
Copy link
Contributor

@jan-vcapgemini jan-vcapgemini commented Oct 23, 2023

Addresses/Fixes: #56

Implements:

  • Jline3 autocompletion
  • some examples
  • test framework
  • some tests
  • jansi for Windows Terminal support
  • checks for invalid input patterns e.g. get-version mvn -

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
@jan-vcapgemini jan-vcapgemini changed the title Feature/56 implement autocompletion #56 implement autocompletion Oct 23, 2023
@jan-vcapgemini jan-vcapgemini self-assigned this Oct 23, 2023
@jan-vcapgemini jan-vcapgemini added the CLI IDEasy command-line-interface (parsing args, etc.) label Oct 23, 2023
added jline license
added jansi license
String word = commandLine.word();
List<String> words = commandLine.words();
// TODO: implement rest of this
Commandlet sub = findSubcommandlet(words, commandLine.wordIndex());
Copy link
Member

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:


As it is a ToolProperty it can only complete to commandlets that are actually instances of ToolCommandlet.
for 3rd: you would find this property:

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.

Copy link
Contributor Author

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/pom.xml 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 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
@jan-vcapgemini
Copy link
Contributor Author

jan-vcapgemini commented Dec 14, 2023

Good work! I have added some comments. Additionally, I have observed, that

* in git bash when type a string `s` and then press `ctrl + c` nothing is printed to the console. Then upon the next key press `k`, `k` is appended to `s` followed by a newline and the msg `User cancled with CTRL+C`.

* In both Windows Terminal and git bash. When I press `ctrl + d` completions are suggested. But when continuing to type the word you want, the completions aren't updated anymore. Test with `set-version k` -> `ctrl + d` -> ` otlincnat` or `set-version az 2.5` -> `ctrl + d` -> `4`

* when running `get-version` on a tool that is not installed, no msg is printed and the prompt is exits. Running Ide directly with this command prints `Tool ... is not installed!`

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
@tobka777 tobka777 assigned hohwille and unassigned MattesMrzik Dec 14, 2023
Copy link
Contributor

@MattesMrzik MattesMrzik left a 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?

@coveralls
Copy link
Collaborator

coveralls commented Dec 15, 2023

Pull Request Test Coverage Report for Build 7462088879

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.5%) to 53.832%

Totals Coverage Status
Change from base Build 7449771963: 0.5%
Covered Lines: 3196
Relevant Lines: 5745

💛 - Coveralls

Copy link
Contributor

@MattesMrzik MattesMrzik left a 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

Comment on lines 152 to 177
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);
}
Copy link
Contributor

@MattesMrzik MattesMrzik Jan 2, 2024

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"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjusted.

Copy link
Member

@hohwille hohwille left a 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.

Comment on lines 136 to 142
// 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");
Copy link
Member

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.

Copy link
Contributor Author

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.

Comment on lines 158 to 164
// 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"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Copy link
Contributor Author

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.

Comment on lines 126 to 129
/**
* Initializes jline3 autocompletion.
*/
private void initializeJlineCompletion() {
Copy link
Member

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjusted.

Comment on lines 113 to 118
try {
initializeJlineCompletion();
return ProcessResult.SUCCESS;
} catch (RuntimeException e) {
return 1;
}
Copy link
Member

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Comment on lines 82 to 89
// 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
Copy link
Member

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).

Copy link
Contributor Author

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.

Comment on lines +90 to +91
toolCommandlets.add(commandlet.getName());
toolCommandlets.add(commandlet.getKeyword());
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not yet I think.

Copy link
Member

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.

Comment on lines +109 to +194
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));
}
}
}
}
}
}
Copy link
Member

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 ....

Copy link
Contributor Author

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.

Comment on lines +209 to +216
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));
}
}
Copy link
Member

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 postFixfeature if we never use it?

Copy link
Contributor Author

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.

Comment on lines 123 to 124
for (IdeLogLevel level : IdeLogLevel.values()) {
IdeSubLogger logger;
Copy link
Member

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjusted now.

@hohwille hohwille assigned jan-vcapgemini and unassigned hohwille Jan 4, 2024
@hohwille hohwille added the completion auto-completion in bash or build in CLI label Jan 4, 2024
@hohwille hohwille added this to the release:2024.01.001 milestone Jan 5, 2024
jan-vcapgemini and others added 10 commits January 8, 2024 12:06
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
@hohwille
Copy link
Member

@jan-vcapgemini thanks again for this great PR. You did solid pioneer work to figure out how to make this work. 👍
Based on your work the following PRs had been created:

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.

@hohwille hohwille closed this Jan 26, 2024
@jan-vcapgemini jan-vcapgemini deleted the feature/56-implement-autocompletion branch May 7, 2024 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI IDEasy command-line-interface (parsing args, etc.) completion auto-completion in bash or build in CLI
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Implement auto-completion
4 participants