feat(complete): group options by tag (in zsh)#6334
Conversation
e5a4b4d to
629cac1
Compare
zsh)zsh)
65f0b45 to
b161c88
Compare
b161c88 to
0609ae5
Compare
zsh)zsh)
| if [[ "$value" == */ ]]; then | ||
| local dir_no_slash="${value%/}" | ||
| if [[ "$completion" == *:* ]]; then | ||
| local desc="${completion#*:}" | ||
| if [[ "$value" == *:* ]]; then | ||
| local desc="${value#*:}" | ||
| dirs+=("$dir_no_slash:$desc") | ||
| else | ||
| dirs+=("$dir_no_slash") | ||
| fi | ||
| else | ||
| other+=("$completion") | ||
| if (( ${+tag_map["$tag"]} )); then # key exists? | ||
| tag_map["$tag"]+=$'\n'"$value" | ||
| else | ||
| tag_map["$tag"]="$value" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Does this mean we aren't getting the / handling if a tag is present?
There was a problem hiding this comment.
The opposite in fact - if a / is there, we'll handle it (since we test that first) and only perform the option grouping for remaining options
There was a problem hiding this comment.
Items with / should still have grouping though
There was a problem hiding this comment.
Thank you, I see what it's for now - fix in place
| } | ||
| write!( | ||
| buf, | ||
| "{}:", |
There was a problem hiding this comment.
I suppose we have two options:
- Pick a different separator - but what if that separator is in the tag?
- Avoid
:in tags, either by filtering them out at this point, or making it part of the tag requirement
What do you think?
There was a problem hiding this comment.
We can pick an unlikely separator, like _SEP_
There was a problem hiding this comment.
Thinking about it, I think we can avoid this entirely - I'll use the escape_value() function which looks to be for precisely this.
eeee9ab to
db855d8
Compare
db855d8 to
9e85b97
Compare
| % zstyle ':completion:*' group-name '' | ||
| % zstyle ':completion:*:descriptions' format '%d' | ||
| % exhaustive - | ||
| "Options" options |
There was a problem hiding this comment.
This is redundant. In --help, we just list the header. We should likely do the same here as well.
There was a problem hiding this comment.
Yes, it was getting my goat a bit too. Fixed, just the header is listed now - the expected output is:
% zstyle ':completion:*:descriptions' format '%d'
% exhaustive -<Tab>
Options <-------------------------- header
--generate -- generate
-h -- Print help
--empty-choice
| "zstyle ':completion:*' group-name ''", | ||
| "zstyle ':completion:*:descriptions' format '%d'", |
There was a problem hiding this comment.
The format style is needed to have zsh show group descriptions - for example:
% ls -<TAB>
# ^ no option completions
% autoload -Uz compinit
% compinit
% ls -<TAB>
-@ -- display extended attribute keys and sizes in long listing
-1 -- single column output
-a -- list entries starting with .
-A -- list all except . and ..
...
# ^ options are shown
% zstyle ':completion:*:descriptions' format '%d'
% ls -<TAB>
option # <----
-@ -- display extended attribute keys and sizes in long listing
-1 -- single column output
-a -- list entries starting with .
-A -- list all except . and ..
...
# ^ note that the "description" header is now shown, in this case a plain "option"(group-name isn't needed so I've dropped it)
There was a problem hiding this comment.
This is a user setting needed to opt-in? Should we document that in the setup instructions?
There was a problem hiding this comment.
Also, should we have the tests opt-in by default?
There was a problem hiding this comment.
This is a user setting needed to opt-in? Should we document that in the setup instructions?
It is - added to the docs, let me know what you think:
//! To show completions grouped by category (options, commands, etc.), also add:
//! ```zsh
//! echo "zstyle ':completion:*:descriptions' format '%d'" >> ~/.zshrc
//! ```
Also, should we have the tests opt-in by default?
I think that would be useful to spot regressions. Would you like me to do that in this PR or a separate one?
(If it helps, I have it ready to go - 1 file changed, 129 insertions(+), 37 deletions(-))
5fea91e to
a9f4522
Compare
| write!(buf, "{}", ifs.as_deref().unwrap_or("\n"))?; | ||
| } | ||
|
|
||
| let tag = format!("{}", candidate.get_tag().unwrap_or(&StyledStr::from(""))); |
There was a problem hiding this comment.
What is this hack addressing and what should it be replaced by?
There was a problem hiding this comment.
Actually, disregard that - it's necessary in order for us to escape the string. We need a &str (rather than a &StyledStr) in order to call escape_value(), which costs us an allocation here.
I've simplified this now, too
97bba1a to
de9a2f5
Compare
| if comp.get_tag().is_some() { | ||
| comp | ||
| } else { | ||
| comp.tag(Some(arg.to_string().into())) |
There was a problem hiding this comment.
Why was this changed?
The intention here is that a positional's value (and maybe an option's value) can have many possible values and it would help the user to name the context we are completing within. Only flags should we default to completing under a header.
There was a problem hiding this comment.
Reverted (this was a foray into a slightly different approach)
| write!(buf, "{}", ifs.as_deref().unwrap_or("\n"))?; | ||
| } | ||
|
|
||
| let tag = format!("{}", candidate.get_tag().unwrap_or(&StyledStr::from(""))); |
There was a problem hiding this comment.
What is this hack addressing and what should it be replaced by?
a189e12 to
0f3a37b
Compare
d8db0b5 to
c1b8c79
Compare
c1b8c79 to
5b6a1ca
Compare
| @@ -275,16 +275,22 @@ fn complete_dynamic_env_quoted_help() { | |||
| let input = "exhaustive quote \t\t"; | |||
| let expected = snapbox::str![[r#" | |||
| % exhaustive quote | |||
There was a problem hiding this comment.
In this test, the --backslash and cmd-backslash have the same description but (I expect) different tags, so I think that's why they're now split
Perhaps we ensure their tags are the same to continue to group them? (if I'm right about the tagging - out of time but I can check later)
There was a problem hiding this comment.
Yes - it's the tags. Options get tagged as "Options" by default:
clap/clap_complete/src/engine/complete.rs
Lines 548 to 553 in c8c9355
and commands similarly:
clap/clap_complete/src/engine/complete.rs
Lines 597 to 602 in c8c9355
To keep the ordering in the tests the same, we could tag commands as options (for the test only):
.help("enum"),
]),
clap::Command::new("quote")
+ .subcommand_help_heading("Options") // group subcommands with options
.args([
clap::Arg::new("single-quotes")
.long("single-quotes")(in clap_complete/examples/exhaustive.rs)
or we could remove the default tag entirely, which is a larger change which would affect not just the tests
This allows zsh to group completions by whether they're global, and then by their tag. For example:
Closes #6320