zsh-workers
 help / color / mirror / code / Atom feed
From: fREW Schmidt <frioux@gmail.com>
To: Oliver Kiddle <okiddle@yahoo.co.uk>
Cc: zsh-workers@zsh.org
Subject: Re: [PATCH] new completion for "sv"
Date: Tue, 7 Oct 2014 08:20:25 -0500	[thread overview]
Message-ID: <20141007132025.GA6252@gae-bulg.lan.mitsi.com> (raw)
In-Reply-To: <5624.1412624240@quattro>


[-- Attachment #1.1: Type: text/plain, Size: 3018 bytes --]

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

[-- Attachment #1.2: 0001-new-completion-for-sv.patch --]
[-- Type: text/plain, Size: 3231 bytes --]

From 05f0fab3a7ddc666c8c0b89fd48b8c05f41b7b52 Mon Sep 17 00:00:00 2001
From: Arthur Axel 'fREW' Schmidt <frioux@gmail.com>
Date: Sun, 5 Oct 2014 14:52:42 -0500
Subject: [PATCH] new completion for "sv"

See http://smarden.org/runit/sv.8.html
---
 Completion/Unix/Command/_sv | 73 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)
 create mode 100644 Completion/Unix/Command/_sv

diff --git a/Completion/Unix/Command/_sv b/Completion/Unix/Command/_sv
new file mode 100644
index 0000000..b2fedc5
--- /dev/null
+++ b/Completion/Unix/Command/_sv
@@ -0,0 +1,73 @@
+#compdef sv
+
+_sv_commands() {
+    local -a sv_ary
+    sv_ary=(
+          'status':'get status of service (and log service if available)'
+              'up':'start if service is running. If service stops, restart'
+            'down':'send SIGTERM and SIGCONT if service is running. After it stops, do not restart'
+            'once':'start if service is not running. Do not restart if it stops'
+           'pause':'send SIGSTOP if service is running'
+            'cont':'send SIGCONT if service is running'
+             'hup':'send SIGHUP if service is running'
+           'alarm':'send SIGALRM if service is running'
+       'interrupt':'send SIGINT if service is running'
+            'quit':'send SIGQUIT if service is running'
+               '1':'send SIGUSR1 if service is running'
+               '2':'send SIGUSR2 if service is running'
+            'term':'send SIGTERM if service is running'
+            'kill':'send SIGKILL if service is running'
+            'exit':'send SIGTERM and SIGCONT if service is running. Do not restart service.'
+    )
+    _describe -t commands "sv commands" sv_ary -V sv_commands
+}
+
+_sv_lsb() {
+    local -a sv_lsb_ary
+    sv_lsb_ary=(
+                'start':'up with check/timeout'
+                 'stop':'down with check/timeout'
+               'reload':'hup with check'
+              'restart':'down and up with check'
+             'shutdown':'exit with check/timeout'
+           'force-stop':'stop with kill on timeout'
+         'force-reload':'reload with kill on timeout'
+        'force-restart':'restart with kill on timeout'
+       'force-shutdown':'shutdown with kill on timeout'
+          'try-restart':'restart if service is already running'
+    )
+
+    _describe -t lsb-commands "sv LSM init compat" sv_lsb_ary -V sv_init_compat
+}
+
+_sv_additional() {
+    _describe -t additional-commands "sv additional commands" '("check:check status of service")' -V sv_addl_comm
+}
+
+local curcontext="$curcontext" state line
+_arguments -C \
+    '-v[verbose]' \
+    '-w[wait time]:time (seconds)' \
+    '1: :->command' \
+    '*:: :->options'
+
+case $state in
+  (command)
+      _alternative \
+         _sv_commands \
+         _sv_lsb \
+         _sv_additional
+  ;;
+
+  (options)
+      local -a sv_services
+      local svdir=${SVDIR:-/service}
+      sv_services=(
+         $svdir/*(N)
+         $svdir/*/log(N)
+      )
+
+      sv_services=( ${sv_services#$svdir/} )
+     _describe -t services "sv services" sv_services
+  ;;
+esac
-- 
2.0.0.390.gcb682f8


[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

  reply	other threads:[~2014-10-07 13:20 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-10-05 20:01 fREW Schmidt
2014-10-05 20:04 ` fREW Schmidt
2014-10-06  9:54   ` Christian Neukirchen
2014-10-06 19:37 ` Oliver Kiddle
2014-10-07 13:20   ` fREW Schmidt [this message]
2014-10-07 15:28     ` Oliver Kiddle
2014-10-07 16:03       ` fREW Schmidt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20141007132025.GA6252@gae-bulg.lan.mitsi.com \
    --to=frioux@gmail.com \
    --cc=okiddle@yahoo.co.uk \
    --cc=zsh-workers@zsh.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/zsh/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).