Oliver Kiddle wrote on Wed, Oct 08, 2014 at 16:35:07 +0200:
> Daniel Shahaf wrote:
> > but I haven't written completion functions before so I might have
> > overlooked something.
> 
> With that in mind, I've taken a look and have a couple of
> recommendations.
> 

Thanks for the review.

> > +local curcontext="$curcontext" ret=1 arguments
> 
> None of these are actually needed. curcontext is only relevant if you
> call _arguments or _values with the -C option. You do use ret but a zsh
> function will pass on the return status of the last command it calls and
> _xxd only adds matches with a final call to _arguments. Finally the
> arguments array is only expanded once and could be specified inline at
> that point.
> 

Okay; I removed 'curcontext' and 'ret'.

As to 'arguments', I don't see a benefit to inlining it, and keeping it as a
separate variable makes the code easier to read.  What would be the benefit of
inlining it at the single use site?

> > +# TODO: permit two hyphens (--autoskip, --groupsize, etc)
> 
> Given that two dashes is accepted but not documented by xxd and that it
> also works for the short options, e.g. --u, I would just strip a dash as
> follows:
> 
>   [[ -prefix -- ]] && compset -P -
> 

Done.

> > +# TODO: xxd -<tab> should show '-x' and '-x:' differently - give visual hint that there's a required argument
> 
> Unless I'm missing something, there's nothing xxd specific in that
> desire. Perhaps it should be considered for the general case. In many

Yes, that's a general issue.  For example, instead of:

    % sudo -<tab>
    -s                                         -- run SHELL
    -u                                         -- user name

I would prefer:

    % sudo -<tab>
    -s                                         -- run SHELL
    -u +                                       -- user name

or even:

    % sudo -<tab>
    -s                                         -- run SHELL
    -u USER                                    -- user name

>

Incidentally, I ran into another general issue:

    % xxd -<tab>
    -z
    -autoskip    -a               -- a single '*' replaces runs of NUL (toggleable
    -bits        -b               -- output in binary digits, rather than hex     
    -cols        -c               -- specify number of octets per line            
    ...

It wasn't immediately obvious to me, but I eventually realized the "-z" in the
output was caused by a stray file named "-z" (via the _files completion defined
for positional arguments).  However, since the filename begins with a hyphen,
specifying it as "-z" won't work as intended.  Perhaps "xxd -<tab>" should
offer "./-z" or "-- -z" (two words) as possible completions?

> cases, you can tell options that take an argument by the fact that the
> description starts with the word "specify". Taking -c as an example, you
> have:
> 
> > +    {-c+,-cols}'[output ARG octets per line]:number of octets per line'
> 
> Until I checked, and found otherwise, I was guessing that the "ARG" came
> from a direct cut and paste from the -help output.
> 

Okay, I switched the descriptions from the "ARG" convention to the "specify"
convention.

> The arguments to -c, -g, -l and -s also get in the way of completing
> -cols, -groupsize, -len and -seek:
>   % xxd -grou<tab>
>   number of octets per group
>   option
>   -groupsize
> 
> To _arguments, the rou characters could be the number of octets. By
> using _guard, we can tell it that the number can only be the characters
> 0-9 (xxd allows hex so a few more characters are allowed there). With
> _guard, it looks like this:
> 
>   {-c+,-cols}'[specify number of octets per line]: :_guard "[0-9a-fA-Fx]#" "number of octets per line"'
> 

Done.  'xxd -grou<tab>' now DTRTs; however, 'xxd -g<tab>' gives me:

    % xxd -g<tab>
    -groupsize  -- specify the number of octets per group

and the display doesn't change even if I press <tab> again.  This seems odd:
why am I not offered both -g and -groupsize as possible completions?  And if
"-groupsize" is offered in the message, why does pressing <tab> not complete
the command-line word to the one possible completion?

> Oliver

Thanks for the tips.  Revised patch attached.

Daniel