-
Notifications
You must be signed in to change notification settings - Fork 418
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
Honor spaces in autocomplete candidates #1759
Conversation
- Both positional params and option candidates are now stored in the autocomplete script as an array. - By setting `IFS=$'\n'` for those scenarios, each array entry is treated as a single candidate, allowing to honor any spaces in the original suggestions coded by the user.
- Actually parse the values into a COMPREPLY array
Thank you for the PR! Checking... |
@remkop any feedback on this? |
Oops! Sorry for the long wait. I have not been able to spend much time on picocli recently... |
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.
LGTM
Merged. |
@remkop glad to help! Any rough timelines for 4.7? |
Started the release process now... |
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.
Hi @jsotuyod! Thank you again for working on this PR!
I realize it is a while ago that you worked on this, but a question recently came up:
@vorburger ran ErrorProne against the picocli project, which flagged some items on AutoComplete.java:788 and 832:
... the format string has only 2
%s
variables, but the code specifies 3 parameters (indent
,paramName
,currWord
)...
This means that the last parameter (currWord
) is not used. Is that correct? Can the currWord
parameter be removed?
@@ -750,7 +785,7 @@ private static String generatePositionalParamsCases(List<PositionalParamSpec> po | |||
int max = param.index().max(); | |||
if (param.completionCandidates() != null) { | |||
buff.append(format("%s %s (( currIndex >= %d && currIndex <= %d )); then\n", indent, ifOrElif, min, max)); | |||
buff.append(format("%s positionals=$( compgen -W \"$%s_pos_param_args\" -- \"%s\" )\n", indent, paramName, currWord)); | |||
buff.append(format("%s positionals=$( compReplyArray \"${%s_pos_param_args[@]}\" )\n", indent, paramName, currWord)); |
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.
We recently discovered that the format string has only 2 %s
variables, but we specify 3 parameters (indent
, paramName
, currWord
)...
This means that the last parameter (currWord
) is not used.
Is that correct? Can the currWord
parameter be removed?
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.
Yeah, it can be removed. Sorry for missing that
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.
fixed in #2175
@@ -793,7 +828,8 @@ private static String generateOptionsCases(List<OptionSpec> argOptionFields, Str | |||
} | |||
if (option.completionCandidates() != null) { | |||
buff.append(format("%s %s)\n", indent, concat("|", option.names()))); // " -u|--timeUnit)\n" | |||
buff.append(format("%s COMPREPLY=( $( compgen -W \"${%s_option_args}\" -- \"%s\" ) )\n", indent, bashify(option.paramLabel()), currWord)); | |||
buff.append(format("%s local IFS=$'\\n'\n", indent)); | |||
buff.append(format("%s COMPREPLY=( $( compReplyArray \"${%s_option_args[@]}\" ) )\n", indent, bashify(option.paramLabel()), currWord)); |
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.
We recently discovered that the format string has only 2 %s
variables, but we specify 3 parameters (indent
, bashify(option.paramLabel())
, currWord
)...
This means that the last parameter (currWord
) is not used.
Is that correct? Can the currWord
parameter be removed?
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.
fixed in #2175
Fixes #1754
IFS=$'\n'
for those scenarios, each array entry is treated as a single candidate,allowing to honor any spaces in the original suggestions coded by the user.