On Mon, Oct 06, 2014 at 09:37:20PM +0200, Oliver Kiddle wrote:
> fREW Schmidt wrote:
> > 
> > If anything is wrong just let me know, this would be my first
> > contribution to zsh so mistakes wouldn't surprise me.
> 
> I've got a few comments.
> 
> > +#compdef sv
> > +
> > +_arguments \
> > +  '-v[verbose]' \
> > +  '-w:wait time' \
> 
> What's the units for the time? How about something like:
>   '-w[specify wait time]:time (seconds)'

Good idea

> > +  ':command:->command' \
> > +  '*::options:->options'
> 
> If you use states with _arguments, you should declare a few array
> variables local: context, state and line. You then need to take care to
> make use of the context variable when calling other functions. In most
> cases, including the one above, only one state is possible at a time. So
> in this case, you should declare:
>   local curcontext="$curcontext" state line
> and pass the -C option to _arguments.

Ok, I think I did this right this time.

> > +    sv_ary=(
> > +           'status':'Get status of service (and log service if available)'
> 
> The usual convention, which you have done elsewhere, is to start
> descriptions with a lowercase letter.

Woops! Fixed.

> > +      _describe -t commands "sv commands" sv_ary -V sv_commands
> > +      _describe -t commands "sv LSM init compat" sv_lsb_ary -V sv_init_compat
> > +      _describe -t commands "sv additional commands" '("check:check status of service")' -V sv_addl_comm
> > +      return
> 
> This is ignoring the return status of the first two calls to _describe.
> The return value of a completion function determines whether the
> completion system carries on trying to find more matches, perhaps
> approximate matching if you have that configured. It's common to use a
> variable named ret for the return status: look for some examples.
> 
> Perhaps also consider using three different tags for the matches and using
> _alternative or _tags with a loop.

ok, I switched it to use _alternative and 3 functions as seems to be
somewhat common in other compltion scripts in the codebase.

> > +      $SVDIR/*(N)
> 
> Is SVDIR really always set or is there a default value that it could
> fall back on, e.g: ${SVDIR:-/service}

This raises an interesting question.  I did what you said, but as
Christian Neukirchen pointed out, on his system the default SVDIR is
/var/service, and on ubuntu the default is /etc/service.  I guess it's
up to the packager to tweak the script or something?

Additionally, I like Christian Neukirchen's single-character shortcut
completion.  For majority of the commands they work already since
nothing else starts with that letter, but s and c both have multiple
options.  If I understood what I was doing more I'd take his idea, but
I don't.

Anyway, see a new version attached.  Let me know what else is (still?)
wrong.

Thanks!

(Again, please CC me in responses.)

-- 
fREW Schmidt
https://blog.afoolishmanifesto.com