Repository: kbknapp/clap-rs
Pull request #1147 (2018-01-14 05:13:50)

Zsh completion improvements
This includes a bunch of improvements to Zsh completion, as separate commits:

1. Complete positional arguments properly

   This changes the way we complete positionals to complete them using `_arguments`, as should be done, instead of completing their uppercase name as a string.

   Currently I made it offer `_files` completion for all positional arguments. This can be improved to complete actual possible values of the arguments and only complete files if the argument truly takes them. But this will require further changes in clap to actually have the required functionality to get this information.

2. Maybe fix completions with mixed positionals and subcommands

   Optional positionals mixed with subcommands will still break this, since I can't see how to tell which element of `$line` is the command than. Mixing optional positionals with subcommands is a bit weird and awkward though...

   This is, of course, a followup of 1 since this wasn't an issue when we weren't completing positionals properly.

3. Don't pass `-S` to _arguments if Zsh is too old

   If you do pass it than `_arguments` considers `-C` as a possible option in the completions.

   This affects Ubuntu 16.04 which uses Zsh 5.1.1 for example.

Related https://github.com/kbknapp/clap-rs/issues/868

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/kbknapp/clap-rs/1147)
<!-- Reviewable:end -->



Patch:
diff --git a/src/completions/zsh.rs b/src/completions/zsh.rs
index c73a9878..11dd868e 100644
--- a/src/completions/zsh.rs
+++ b/src/completions/zsh.rs
@@ -31,10 +31,19 @@ impl<'a, 'b> ZshGen<'a, 'b> {
                 "\
 #compdef {name}
 
+autoload -U is-at-least
+
 _{name}() {{
     typeset -A opt_args
+    typeset -a _arguments_options
     local ret=1
 
+    if is-at-least 5.2; then
+        _arguments_options=(-s -S -C)
+    else
+        _arguments_options=(-s -C)
+    fi
+
     local context curcontext=\"$curcontext\" state line
     {initial_args}
     {subcommands}
@@ -52,7 +61,7 @@ _{name} \"$@\"",
     }
 }
 
-// Displays the positional args and commands of a subcommand
+// Displays the commands of a subcommand
 // (( $+functions[_[bin_name_underscore]_commands] )) ||
 // _[bin_name_underscore]_commands() {
 // 	local commands; commands=(
@@ -63,8 +72,8 @@ _{name} \"$@\"",
 // Where the following variables are present:
 //    [bin_name_underscore]: The full space deliniated bin_name, where spaces have been replaced by
 //                           underscore characters
-//    [arg_name]: The name of the positional arg or subcommand
-//    [arg_help]: The help message of the arg or subcommand
+//    [arg_name]: The name of the subcommand
+//    [arg_help]: The help message of the subcommand
 //    [bin_name]: The full space deliniated bin_name
 //
 // Here's a snippet from rustup:
@@ -94,7 +103,7 @@ _{bin_name_underscore}_commands() {{
 }}",
             bin_name_underscore = p.meta.bin_name.as_ref().unwrap().replace(" ", "__"),
             bin_name = p.meta.bin_name.as_ref().unwrap(),
-            subcommands_and_args = subcommands_and_args_of(p)
+            subcommands_and_args = subcommands_of(p)
         ),
     ];
 
@@ -115,26 +124,26 @@ _{bin_name_underscore}_commands() {{
 }}",
             bin_name_underscore = bin_name.replace(" ", "__"),
             bin_name = bin_name,
-            subcommands_and_args = subcommands_and_args_of(parser_of(p, bin_name))
+            subcommands_and_args = subcommands_of(parser_of(p, bin_name))
         ));
     }
 
     ret.join("\n")
 }
 
-// Generates subcommand and positional argument completions in form of
+// Generates subcommand completions in form of
 //
 // 		'[arg_name]:[arg_help]'
 //
 // Where:
-//    [arg_name]: the argument or subcommand's name
-//    [arg_help]: the help message of the argument or subcommand
+//    [arg_name]: the subcommand's name
+//    [arg_help]: the help message of the subcommand
 //
 // A snippet from rustup:
 // 		'show:Show the active and installed toolchains'
 //      'update:Update Rust toolchains'
-fn subcommands_and_args_of(p: &Parser) -> String {
-    debugln!("ZshGen::subcommands_and_args_of;");
+fn subcommands_of(p: &Parser) -> String {
+    debugln!("ZshGen::subcommands_of;");
     let mut ret = vec![];
     fn add_sc(sc: &App, n: &str, ret: &mut Vec<String>) {
         debugln!("ZshGen::add_sc;");
@@ -153,10 +162,10 @@ fn subcommands_and_args_of(p: &Parser) -> String {
         }
     }
 
-    // First the subcommands
+    // The subcommands
     for sc in p.subcommands() {
         debugln!(
-            "ZshGen::subcommands_and_args_of:iter: subcommand={}",
+            "ZshGen::subcommands_of:iter: subcommand={}",
             sc.p.meta.name
         );
         add_sc(sc, &sc.p.meta.name, &mut ret);
@@ -167,24 +176,6 @@ fn subcommands_and_args_of(p: &Parser) -> String {
         }
     }
 
-    // Then the positional args
-    for arg in p.positionals() {
-        debugln!("ZshGen::subcommands_and_args_of:iter: arg={}", arg.b.name);
-        let a = format!(
-            "\"{name}:{help}\" \\",
-            name = arg.b.name.to_ascii_uppercase(),
-            help = arg.b
-                .help
-                .unwrap_or("")
-                .replace("[", "\\[")
-                .replace("]", "\\]")
-        );
-
-        if !a.is_empty() {
-            ret.push(a);
-        }
-    }
-
     ret.join("\n")
 }
 
@@ -248,15 +239,18 @@ fn get_subcommands_of(p: &Parser) -> String {
     format!(
         "case $state in
     ({name})
-        curcontext=\"${{curcontext%:*:*}}:{name_hyphen}-command-$words[1]:\"
-        case $line[1] in
+        words=($line[{pos}] \"${{words[@]}}\")
+        (( CURRENT += 1 ))
+        curcontext=\"${{curcontext%:*:*}}:{name_hyphen}-command-$line[{pos}]:\"
+        case $line[{pos}] in
             {subcommands}
         esac
     ;;
 esac",
         name = p.meta.name,
         name_hyphen = p.meta.bin_name.as_ref().unwrap().replace(" ", "-"),
-        subcommands = subcmds.join("\n")
+        subcommands = subcmds.join("\n"),
+        pos = p.positionals().len() + 1
     )
 }
 
@@ -268,8 +262,8 @@ fn parser_of<'a, 'b>(p: &'b Parser<'a, 'b>, sc: &str) -> &'b Parser<'a, 'b> {
     &p.find_subcommand(sc).expect(INTERNAL_ERROR_MSG).p
 }
 
-// Writes out the args section, which ends up being the flags and opts, and a jump to
-// another ZSH function if there are positional args or subcommands.
+// Writes out the args section, which ends up being the flags, opts and postionals, and a jump to
+// another ZSH function if there are subcommands.
 // The structer works like this:
 //    ([conflicting_args]) [multiple] arg [takes_value] [[help]] [: :(possible_values)]
 //       ^-- list '-v -h'    ^--'*'          ^--'+'                   ^-- list 'one two three'
@@ -280,8 +274,8 @@ fn parser_of<'a, 'b>(p: &'b Parser<'a, 'b>, sc: &str) -> &'b Parser<'a, 'b> {
 // 		'(-h --help --verbose)-v[Enable verbose output]' \
 // 		'(-V -v --version --verbose --help)-h[Prints help information]' \
 //      # ... snip for brevity
-// 		'1:: :_rustup_commands' \   # <-- displays positional args and subcommands
-// 		'*:: :->rustup' \           # <-- displays subcommand args and child subcommands
+// 		':: :_rustup_commands' \    # <-- displays subcommands
+// 		'*::: :->rustup' \          # <-- displays subcommand args and child subcommands
 // 	&& ret=0
 //
 // The args used for _arguments are as follows:
@@ -290,19 +284,20 @@ fn parser_of<'a, 'b>(p: &'b Parser<'a, 'b>, sc: &str) -> &'b Parser<'a, 'b> {
 //    -S: Do not complete anything after '--' and treat those as argument values
 fn get_args_of(p: &Parser) -> String {
     debugln!("get_args_of;");
-    let mut ret = vec![String::from("_arguments -s -S -C \\")];
+    let mut ret = vec![String::from("_arguments \"${_arguments_options[@]}\" \\")];
     let opts = write_opts_of(p);
     let flags = write_flags_of(p);
-    let sc_or_a = if p.has_subcommands() || p.has_positionals() {
+    let positionals = write_positionals_of(p);
+    let sc_or_a = if p.has_subcommands() {
         format!(
-            "\"1:: :_{name}_commands\" \\",
+            "\":: :_{name}_commands\" \\",
             name = p.meta.bin_name.as_ref().unwrap().replace(" ", "__")
         )
     } else {
         String::new()
     };
     let sc = if p.has_subcommands() {
-        format!("\"*:: :->{name}\" \\", name = p.meta.name)
+        format!("\"*::: :->{name}\" \\", name = p.meta.name)
     } else {
         String::new()
     };
@@ -313,6 +308,9 @@ fn get_args_of(p: &Parser) -> String {
     if !flags.is_empty() {
         ret.push(flags);
     }
+    if !positionals.is_empty() {
+        ret.push(positionals);
+    }
     if !sc_or_a.is_empty() {
         ret.push(sc_or_a);
     }
@@ -324,8 +322,8 @@ fn get_args_of(p: &Parser) -> String {
     ret.join("\n")
 }
 
-// Escape string inside single quotes and brackets
-fn escape_string(string: &str) -> String {
+// Escape help string inside single quotes and brackets
+fn escape_help(string: &str) -> String {
     string
         .replace("\\", "\\\\")
         .replace("'", "'\\''")
@@ -333,12 +331,22 @@ fn escape_string(string: &str) -> String {
         .replace("]", "\\]")
 }
 
+// Escape value string inside single quotes and parentheses
+fn escape_value(string: &str) -> String {
+    string
+        .replace("\\", "\\\\")
+        .replace("'", "'\\''")
+        .replace("(", "\\(")
+        .replace(")", "\\)")
+        .replace(" ", "\\ ")
+}
+
 fn write_opts_of(p: &Parser) -> String {
     debugln!("write_opts_of;");
     let mut ret = vec![];
     for o in p.opts() {
         debugln!("write_opts_of:iter: o={}", o.name());
-        let help = o.help().map_or(String::new(), escape_string);
+        let help = o.help().map_or(String::new(), escape_help);
         let mut conflicts = get_zsh_arg_conflicts!(p, o, INTERNAL_ERROR_MSG);
         conflicts = if conflicts.is_empty() {
             String::new()
@@ -352,7 +360,8 @@ fn write_opts_of(p: &Parser) -> String {
             ""
         };
         let pv = if let Some(pv_vec) = o.possible_vals() {
-            format!(": :({})", pv_vec.join(" "))
+            format!(": :({})", pv_vec.iter().map(
+                |v| escape_value(*v)).collect::<Vec<String>>().join(" "))
         } else {
             String::new()
         };
@@ -392,7 +401,7 @@ fn write_flags_of(p: &Parser) -> String {
     let mut ret = vec![];
     for f in p.flags() {
         debugln!("write_flags_of:iter: f={}", f.name());
-        let help = f.help().map_or(String::new(), escape_string);
+        let help = f.help().map_or(String::new(), escape_help);
         let mut conflicts = get_zsh_arg_conflicts!(p, f, INTERNAL_ERROR_MSG);
         conflicts = if conflicts.is_empty() {
             String::new()
@@ -434,3 +443,30 @@ fn write_flags_of(p: &Parser) -> String {
 
     ret.join("\n")
 }
+
+fn write_positionals_of(p: &Parser) -> String {
+    debugln!("write_positionals_of;");
+    let mut ret = vec![];
+    for arg in p.positionals() {
+        debugln!("write_positionals_of:iter: arg={}", arg.b.name);
+        let a = format!(
+            "'{optional}:{name}{help}:{action}' \\",
+            optional = if !arg.b.is_set(ArgSettings::Required) { ":" } else { "" },
+            name = arg.b.name,
+            help = arg.b
+                .help
+                .map_or("".to_owned(), |v| " -- ".to_owned() + v)
+                .replace("[", "\\[")
+                .replace("]", "\\]"),
+            action = arg.possible_vals().map_or("_files".to_owned(), |values| {
+                format!("({})",
+                    values.iter().map(|v| escape_value(*v)).collect::<Vec<String>>().join(" "))
+            })
+        );
+
+        debugln!("write_positionals_of:iter: Wrote...{}", a);
+        ret.push(a);
+    }
+
+    ret.join("\n")
+}


Test patch:
diff --git a/tests/completions.rs b/tests/completions.rs
index 84018c8b..f25f9429 100644
--- a/tests/completions.rs
+++ b/tests/completions.rs
@@ -89,25 +89,37 @@ complete -F _myapp -o bashdefault -o default myapp
 
 static ZSH: &'static str = r#"#compdef myapp
 
+autoload -U is-at-least
+
 _myapp() {
     typeset -A opt_args
+    typeset -a _arguments_options
     local ret=1
 
+    if is-at-least 5.2; then
+        _arguments_options=(-s -S -C)
+    else
+        _arguments_options=(-s -C)
+    fi
+
     local context curcontext="$curcontext" state line
-    _arguments -s -S -C \
+    _arguments "${_arguments_options[@]}" \
 '-h[Prints help information]' \
 '--help[Prints help information]' \
 '-V[Prints version information]' \
 '--version[Prints version information]' \
-"1:: :_myapp_commands" \
-"*:: :->myapp" \
+'::file -- some input file:_files' \
+":: :_myapp_commands" \
+"*::: :->myapp" \
 && ret=0
     case $state in
     (myapp)
-        curcontext="${curcontext%:*:*}:myapp-command-$words[1]:"
-        case $line[1] in
+        words=($line[2] "${words[@]}")
+        (( CURRENT += 1 ))
+        curcontext="${curcontext%:*:*}:myapp-command-$line[2]:"
+        case $line[2] in
             (test)
-_arguments -s -S -C \
+_arguments "${_arguments_options[@]}" \
 '--case=[the case to test]' \
 '-h[Prints help information]' \
 '--help[Prints help information]' \
@@ -116,7 +128,7 @@ _arguments -s -S -C \
 && ret=0
 ;;
 (help)
-_arguments -s -S -C \
+_arguments "${_arguments_options[@]}" \
 '-h[Prints help information]' \
 '--help[Prints help information]' \
 '-V[Prints version information]' \
@@ -133,7 +145,6 @@ _myapp_commands() {
     local commands; commands=(
         "test:tests things" \
 "help:Prints this message or the help of the given subcommand(s)" \
-"FILE:some input file" \
     )
     _describe -t commands 'myapp commands' commands "$@"
 }
@@ -378,25 +389,37 @@ static POWERSHELL_SPECIAL_CMDS: &'static str = r#"
 
 static ZSH_SPECIAL_CMDS: &'static str = r#"#compdef my_app
 
+autoload -U is-at-least
+
 _my_app() {
     typeset -A opt_args
+    typeset -a _arguments_options
     local ret=1
 
+    if is-at-least 5.2; then
+        _arguments_options=(-s -S -C)
+    else
+        _arguments_options=(-s -C)
+    fi
+
     local context curcontext="$curcontext" state line
-    _arguments -s -S -C \
+    _arguments "${_arguments_options[@]}" \
 '-h[Prints help information]' \
 '--help[Prints help information]' \
 '-V[Prints version information]' \
 '--version[Prints version information]' \
-"1:: :_my_app_commands" \
-"*:: :->my_app" \
+'::file -- some input file:_files' \
+":: :_my_app_commands" \
+"*::: :->my_app" \
 && ret=0
     case $state in
     (my_app)
-        curcontext="${curcontext%:*:*}:my_app-command-$words[1]:"
-        case $line[1] in
+        words=($line[2] "${words[@]}")
+        (( CURRENT += 1 ))
+        curcontext="${curcontext%:*:*}:my_app-command-$line[2]:"
+        case $line[2] in
             (test)
-_arguments -s -S -C \
+_arguments "${_arguments_options[@]}" \
 '--case=[the case to test]' \
 '-h[Prints help information]' \
 '--help[Prints help information]' \
@@ -405,7 +428,7 @@ _arguments -s -S -C \
 && ret=0
 ;;
 (some_cmd)
-_arguments -s -S -C \
+_arguments "${_arguments_options[@]}" \
 '--config=[the other case to test]' \
 '-h[Prints help information]' \
 '--help[Prints help information]' \
@@ -414,7 +437,7 @@ _arguments -s -S -C \
 && ret=0
 ;;
 (some-cmd-with-hypens)
-_arguments -s -S -C \
+_arguments "${_arguments_options[@]}" \
 '-h[Prints help information]' \
 '--help[Prints help information]' \
 '-V[Prints version information]' \
@@ -422,7 +445,7 @@ _arguments -s -S -C \
 && ret=0
 ;;
 (help)
-_arguments -s -S -C \
+_arguments "${_arguments_options[@]}" \
 '-h[Prints help information]' \
 '--help[Prints help information]' \
 '-V[Prints version information]' \
@@ -441,7 +464,6 @@ _my_app_commands() {
 "some_cmd:tests other things" \
 "some-cmd-with-hypens:" \
 "help:Prints this message or the help of the given subcommand(s)" \
-"FILE:some input file" \
     )
     _describe -t commands 'my_app commands' commands "$@"
 }
@@ -655,12 +677,21 @@ complete -c my_app -n "__fish_using_command my_app" -s V -l version -d 'Prints v
 
 static ZSH_SPECIAL_HELP: &'static str = r#"#compdef my_app
 
+autoload -U is-at-least
+
 _my_app() {
     typeset -A opt_args
+    typeset -a _arguments_options
     local ret=1
 
+    if is-at-least 5.2; then
+        _arguments_options=(-s -S -C)
+    else
+        _arguments_options=(-s -C)
+    fi
+
     local context curcontext="$curcontext" state line
-    _arguments -s -S -C \
+    _arguments "${_arguments_options[@]}" \
 '--single-quotes[Can be '\''always'\'', '\''auto'\'', or '\''never'\'']' \
 '--double-quotes[Can be "always", "auto", or "never"]' \
 '--backticks[For more information see `echo test`]' \