Re: Update _virsh completion

[Oliver Kiddle <okiddle@xxxxxxxxxxx>]

Daniel Shahaf wrote:
> Marko Myllynen wrote on Mon, Aug 29, 2016 at 11:03:13 +0300:
> > The patch below (almost) completes _virsh completions:

Thanks. I committed it. Daniel has covered most of the points I wanted to
make.

One thing I found relating to virsh, having checked a system that
actually uses libvirt is that there was some virtual machines that
were listed with --inactive but not with --state-shutoff. virsh start
was able to start them.

> > $ virsh start --domain foo --autodestroy <TAB>
> > --autodestroy   --console       --pass-fds                                  
>
> That's because the expansion contains "--autodestroy" twice.  You can
> easily uniquify the expansion with ${(u)foo} (or 'typeset -aU').

I think it is caused by the presence of the word foo on the
line which is not one of the values passed to _values. I'd be inclined
to use _arguments instead of _values and have it handle things like
--domain taking and argument.

For a number of the other new uses of _values, _wanted would be
sufficient. It may be longer to write "_wanted tag expl desc compadd
…" than "_values desc …" but it does a lot less.  If you do stick
with _values, the convention for descriptions is not to use plural,
e.g server instead of servers. The description describes what should
be entered (e.g. a server) not what the listed matches are (e.g.
servers).

> > -    if (( ! $+_cache_virsh_cmdopts )); then
> > -      typeset -gA _cache_virsh_cmdopts
> > +    if (( ! $+_cache_virsh_cmd_opts )); then
> > +      typeset -gA _cache_virsh_cmd_opts
>
> There's a cache mechanism in compsys, see the 'use-cache' style.
> However, I don't know whether it would make sense to use it here.

That only really makes sense when generating the matches is particularly
slow which isn't the case here.

Oliver