Re: [PATCH] new completion for "sv"

[Oliver Kiddle <okiddle@xxxxxxxxxxx>]

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)'

> +  ':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.

> +    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.

> +      _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.

> +      $SVDIR/*(N)

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

Oliver