Re: [PATCH] _virsh (Was: Re: zsh virsh completion)

[Daniel Shahaf <d.s@daniel.shahaf.name>]

Resubjecting to draw attention to your patch.

I haven't done a full review, but I did spot a couple of trivial issues:

Marko Myllynen wrote on Mon, Jul 18, 2016 at 15:06:52 +0300:
>  case $service in
>    virsh)
>      if (( ! $+_cache_virsh_cmds )); then
>        _cache_virsh_cmds=( ${${${${(f):-"$(_call_program options virsh help)"}:#*:}/# ##}/ *} )
> +      for icmd in $interact_cmds; do

Need to declare $icmd local.

> +        _cache_virsh_cmds[$_cache_virsh_cmds[(i)$icmd]]=()
> +      done
>      fi
>      if (( ! $+_cache_virsh_cmdopts )); then
>        typeset -gA _cache_virsh_cmdopts
>      fi
>      _arguments -A "-*" -C -S -s -w \

Oliver remarked on the -w earlier, are you quite sure it's correct?

              -w     In combination with -s, allow option stacking even if one
                     or more of the options take arguments.  For example, if
                     -x takes an argument, with no -s, `-xy' is considered as
                     a single (unhandled) option; with -s, -xy is an option
                     with the argument `y'; with both -s and -w, -xy may be
                     the option -x and the option -y with arguments still to
                     come.

(-s and -w are options to _arguments, -xy is a word on the command line
being completed)

> -      '(-c --connect)'{-c+,--connect}'[specify connection URI]:URI:_hosts' \

Should be {-c+,--connect=} with an equals sign?

Cheers,

Daniel