#1392: smart completions#1999
Conversation
- Add AutoCompletionRegistry - Add ToolArgumentsProperty - Delegate tool argument completion to ToolCommandlet - Add initial Maven completion entries - Keep default registry empty
Coverage Report for CI Build 27616222521Coverage increased (+0.04%) to 71.325%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions107 previously-covered lines in 6 files lost coverage.
Coverage Stats💛 - Coveralls |
AdemZarrouki
left a comment
There was a problem hiding this comment.
Nice Job. You extended the auto complition for mvn.
|
When testing with GraalVM using ide shell, i typed ide mvn -Dexec.m and then pressed tab but the shell was not responding anymore. @Caylipp can you check the cause of this. Aslo tried to run it in a junit test but the test was running for ever |
…vonfw#1974) Co-authored-by: Jörg Hohwiller <hohwille@users.noreply.github.com>
Co-authored-by: Jörg Hohwiller <hohwille@users.noreply.github.com>
Thanks for pointing this out. |
|
I tested the new fix and the endless loop wasn't present anymore. The only problem is that in the issue its stated that |
- add fix into functions
You’re right, thanks for pointing this out. |
-add comments
|
Also fixed completion for |
AdemZarrouki
left a comment
There was a problem hiding this comment.
Good Job @Caylipp. You fixed the issue. I left a small comment regarding line break in javadoc that need to be removed
699b08e to
f7e0575
Compare
hohwille
left a comment
There was a problem hiding this comment.
@Caylipp thanks for your PR. Looks really nice now. Great work 👍
I have to admit that I do not fully understand the workaround for the = handling.
Is that maybe because = is by default in COMP_WORDBREAKS?
I guess this is for situations where we have --option=val[TAB] and want to get --option=value. So what is by default braking here? Is a space added before or directly after the equals sign?
Then I get at least the compopt -o nospace workaround.
| if (!matchedOption) { | ||
| if (valueIterator.hasNext()) { | ||
| Property<?> valueProperty = valueIterator.next(); | ||
| boolean success = valueProperty.apply(arguments, this, cmd, collector); | ||
| if (!success) { | ||
| LOG.trace("Completion cannot match option or value."); | ||
| } | ||
| } else { | ||
| LOG.trace("No value left for completion."); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
Looks like copy&paste of code (from outer else block). Could be avoided by taking the original code out of the else and add a separate condition by the new flag.
However, is it correct to complete from valueIterator if we are inside if-condition currentArgument.isOption()? This code does not really make sense to me.
In case I am missing something, please let me know...
There was a problem hiding this comment.
I now guess the problem is that for mvn you want to complete options like -DskipTests as part of the ToolArgumentsProperty.
If that is the correct understanding, then IMHO the proper fix would be that ToolArgumentsProperty should implicitly trigger endOptions() on CliArguments.
There was a problem hiding this comment.
For multi-valued properties like here we have this:
IDEasy/cli/src/main/java/com/devonfw/tools/ide/context/AbstractIdeContext.java
Lines 1577 to 1579 in a4f2516
However, in this code:
Wo do check for !isEndOptions() but not for isSplitShortOpts().
There was a problem hiding this comment.
Maybe in this multi-valued property scenario we should call endOptions() instead of stopSplitShortOptions()?
There was a problem hiding this comment.
Thanks, that makes sense.
The fallback was introduced to handle cases like -Dexec..., which are treated as options due to the leading -, but are actually tool arguments. So if no matching IDE option is found, the fallback allows completing them via the value logic. But I agree that this is not a cleanest solution, as it mixes option and value handling and duplicates logic from the value branch. Your suggestion makes more sense: treating ToolArgumentsProperty explicitly as values by triggering endOptions() avoids the need for the fallback. I'll rework the solution in that direction.
There was a problem hiding this comment.
I refactored this now so we no longer jump from option completion into value completion directly.
Instead, the code now uses isEndOptions() to stop option parsing when a value property requires it. This way, tool arguments like -Dexec.* are handled by the normal value completion flow.
| COMPREPLY=( $(ideasy -q complete ${COMP_WORDS[@]:1}) ) | ||
| fi | ||
|
|
||
| # keywords ending with "=" remove whitespace |
There was a problem hiding this comment.
| # keywords ending with "=" remove whitespace | |
| # keywords ending with "=" remove whitespace |
There was a problem hiding this comment.
I updated the comment.
| } | ||
|
|
||
| # remove ":" from word breaks for correct completion | ||
| if [ -n "${BASH_VERSION:-}" ]; then |
There was a problem hiding this comment.
I do not understand the condition. Should this not run in zsh?
I would rather expect something like this:
| if [ -n "${BASH_VERSION:-}" ]; then | |
| if [ -n "${COMP_WORDBREAKS}" ]; then |
If I misunderstood, then maybe you could improve the comment above.
There was a problem hiding this comment.
Good point. I used BASH_VERSION because I considered COMP_WORDBREAKS to be Bash specific and wanted to make sure the workaround only runs in Bash.
I did not consider the zsh case here. But checking COMP_WORDBREAKS directly makes more sense here.
There was a problem hiding this comment.
I updated the condition to check COMP_WORDBREAKS directly instead of BASH_VERSION.
The whitespace issue is not caused by In case of |
…into feature/1392-smart-completions
|
I also noticed one additional edge case while retesting: |
|
I did some debugging on this. For
|
This PR fixes #1392
Implemented changes:
AutoCompletionRegistryfor tool-specific completion candidatesToolArgumentsPropertyfor completing tool argumentsToolArgumentsPropertytoToolCommandletToolCommandletMvnTesting instructions
Please add conscise, understandable instructions on how a reviewer can test/verify the functionality of your contribution here:
Additionally, you can use the end-to-end testing script to test the completion natively in your terminal (#2011).
Checklist for this PR
Make sure everything is checked before merging this PR. For further info please also see
our DoD.
mvn clean testlocally all tests pass and build is successful#«issue-id»: «brief summary»(e.g.#921: fixed setup.bat). If no issue ID exists, title only.In Progressand assigned to you or there is no issue (might happen for very small PRs)with
internal