#1392: Smart completions#1859
Conversation
HenokLachmann
added
completion
HenokLachmann
force-pushed
the
feature/#1392-smart-completions
branch
from
499ecbe to
d4ae88b
Compare
Coverage Report for CI Build 26101908636Coverage increased (+0.003%) to 70.982%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions240 previously-covered lines in 23 files lost coverage.
Coverage Stats
💛 - Coveralls |
tineff96
left a comment
tineff96
left a comment
There was a problem hiding this comment.
Reviewed and tested.
- mvn clean test passed
- verified mvn CLI behavior and completion
The changes look solid and work as expected.
I’m still relatively new to the project, but everything seems correct from my side.
Looks good to me.
hohwille
left a comment
hohwille
left a comment
There was a problem hiding this comment.
@HenokLachmann thanks for this PR and adding great improvements for auto-completion 👍
However, during the review I found several problems where we will need some rework and partially also change the design.
Since you are not available any more, I suggest we will discuss with the team in the next daily how to proceed so that someone else from the team can take over so we can bring this to a merge-able state.
| * @param commandlet the {@link Commandlet} owning the {@link Property}. | ||
| */ | ||
| void add(String text, String description, Property<?> property, Commandlet commandlet); | ||
| void add(String text, String description); |
There was a problem hiding this comment.
IMHO you were mislead by IntelliJ linting that is sometimes stupid.
Because the implementation of this method is (currently) not using these 2 arguments does not automatically mean that we should remove them.
This was designed for the feature in the ide shell where we can use these parameters to automatically derive the localised help texts and display them together with the auto-completion suggestions in the tailtip widget.
However, this story was never completed when @jan-vcapgemini left our team.
| return this.multivalued; | ||
| } | ||
|
|
||
| public boolean isPlaceholder() { |
There was a problem hiding this comment.
Can you please add JavaDoc so we all know and understand what this means?
I can somehow conclude that this is for properties where the value can be autocompleted.
BTW: The better pattern to introduce such method is to better not add a field and a constructor argument.
If I am reading your change properly most sub-classes are not affected and never need the dynamic field.
Wouldn't it be better and easier to do the following instead:
/**
* @return {@code true} if the property value is a placeholder that can be auto-completed.
*/
public boolean isPlaceholder() {
return false;
}
Then you can override it in MvnArgProperty or other sublcasses typically like this:
@Override
public boolean isPlaceholder() {
return true;
}
However, I am not yet convinced that we need this method at all.
| this.alsoMake = this.add(new FlagProperty("--also-make", false, "-am")); | ||
| this.alsoMakeDependents = this.add(new FlagProperty("--also-make-dependents", false, "-amd")); | ||
| this.failAtEnd = this.add(new FlagProperty("--fail-at-end", false, "-fae")); | ||
| this.failFast = this.add(new FlagProperty("--fail-fast", false, "-ff")); |
There was a problem hiding this comment.
This will not work. Our CLI parser is compatible with the POSIX standard.
Therefore -ftb is a short-cut for -f -t -b.
With this change the end-user will never be able to use these POSIX incompatible "short options" for maven any more or in other words:
Currently ide mvn -fae install is the same as mvn -fae install but after this change it will fail with with such error message what will totally confuse the end-user (since mvn -f -a -e install was actually called):
POM file -a specified with the -f/--file command line argument does not exist
| opt.--also-make=Build projects required by the list. | ||
| opt.--also-make-dependents=Build projects that depend on projects on the list. | ||
| opt.--batch=enable batch mode (non-interactive). | ||
| opt.--code=clone given code repository containing a settings folder into workspaces so that settings can be committed alongside code changes. | ||
| opt.--debug=enable debug logging. | ||
| opt.--define\ deployAtEnd=Deploy all artifacts at the end of the multi-module build. | ||
| opt.--define\ exec.args\==Arguments for the exec:java plugin. | ||
| opt.--define\ exec.mainClass\==Main class for the exec:java plugin. | ||
| opt.--define\ skipTests=Skip test compilation and execution. | ||
| opt.--fail-at-end=Only fail the build at the end; allow all non-impacted builds to continue. | ||
| opt.--fail-fast=Stop at first failure in reactorized builds. |
There was a problem hiding this comment.
IMHO we should not start adding and maintaining all the options of all commandlets that we integrate.
This will open the door to hell.
|
|
||
| // assert | ||
| assertThat(context).logAtDebug().hasMessage("Step 'ide' ended with failure."); | ||
| assertThat(context).logAtDebug().hasMessage("Step 'ide' ended successfully."); |
There was a problem hiding this comment.
This looks correct to me:
$ cd /
$ ide --debug env
Running commandlet EnvironmentCommandlet[env]
...
Step 'ide' ended successfully.
But why it this related to #1392?
I ran the test on main and got
The environment variable IDE_ROOT is undefined. Please reinstall IDEasy or manually repair IDE_ROOT variable.
Step 'ide' ended with failure.
Funny, so it seems you actually fixed something that was rather odd on main.
I am still curious to find out what change in this PR changed the IDE_ROOT error to go away.
| protected void setFirstKeyword(String keyword, String alias) { | ||
| KeywordProperty property = new KeywordProperty(keyword, true, alias); | ||
|
|
||
| this.firstKeyword = property; | ||
| this.add(property); | ||
| } | ||
|
|
||
| /** | ||
| * Create a new keyword property without an alias and set it as the first keyword property. | ||
| * | ||
| * @param keyword the string to create the property from | ||
| */ | ||
| protected void setFirstKeyword(String keyword) { | ||
| this.setFirstKeyword(keyword, null); | ||
| } |
There was a problem hiding this comment.
we are setting the firstKeyword automatically and implicitly by addKeyword method here:
IDEasy/cli/src/main/java/com/devonfw/tools/ide/commandlet/Commandlet.java
Lines 105 to 115 in 3f2de9e
This guarantees that our properties are ordered and firstKeyword is also the first entry in our propertiesList that is an instanceof KeywordProperty.
When we use this method we are betraying that contract and IMHO cause a slightly inconsistent situation.
Isn't that odd and to be avoided?
4 participants
This PR fixes #1392
Implemented changes:
Auto-completion is extended for the
Mvncommandlet. It enables autocompletion for theShellCommandletas well as for bash. Completion is supported for the last word as well as intermittent words on a commandline. Completion covers the following:clean,install,package--fail-fast-Dexec.mainClass=<classname>Whitespace is excluded after a completed trailing
=by differentiating partial from full completions. Merged short options are also supported as they are for present options.Completion is based on
Propertyinstances, but properties are also used as stand-in replacements. Examples are therepositoryproperty forRepositoryCommandletor thecommandletproperty forHelpCommandlet. These are not literal, they should not participate in autocompletion. A new boolean propertyplaceholderis introduced to exclude them from completion.A bug existed in the current implementation that would complete the
exitsubcommand infinitely. This is fixed by this PR by makingexitan ordinaryKeywordPropertyand dynamically adding it when a shell session is started.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