Re: [patch] Completions for cu, fw_update, and rcctl

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

Matthew Martin wrote on Tue, Jan 12, 2016 at 22:58:53 -0600:
> On Sun, Jan 10, 2016 at 07:53:22PM +0000, Daniel Shahaf wrote:
> > Matthew Martin wrote on Sun, Jan 10, 2016 at 01:51:41 -0600:
> > > Few completers for OpenBSD utilities that have been sitting in my tree.
> > > 
> > > Not sure if listing line speeds is overkill (fine by me to remove them
> > > if anyone thinks it is).
> > 
> > I think it's a nice touch.  When I work with cu the last thing I want is
> > to have to remember whether the magic number is 115200 or 112500 ???
> > 
> > > diff --git a/Completion/BSD/Command/_cu b/Completion/BSD/Command/_cu
> > > new file mode 100644
> > > index 0000000..d4658e3
> > > --- /dev/null
> > > +++ b/Completion/BSD/Command/_cu
> > > @@ -0,0 +1,7 @@
> > > +#compdef cu
> > > +
> > > +_arguments -s -A '-*' \
> > > +  '-d[do not block waiting for a carrier to be detected]' \
> > > +  '-l[line to use]:line:(/dev/cuaU#<->(%))' \
> > 
> > I'm surprised a glob pattern works inside the parentheses.  I'd have
> > expected a call to _files would be needed.
> > 
> > On Linux the device is called /dev/ttyS0, so the pattern could be:
> >     /dev/(cuaU#<->|ttyS<->)(N%c)
> > ? [with (N) to support other OSes]
> > 
> > Maybe encapsulate this as Completion/Unix/Type/_tty_lines so screen(1)
> > and friends can reuse it in the future?
> 
> Like so? Made a _tty_speeds too.

Thanks for going through the existing completion functions!

> +++ b/Completion/Unix/Command/_gdb
> @@ -37,9 +37,7 @@ else
> -  (-b)     _wanted -V values expl 'baud rate' \
> -               compadd 0 50 75 110 134 150 200 300 600 1200 1800 2400 4800 \
> -		       9600 19200 38400 57600 115200 230400 && return 0 ;;
> +  (-b)     _tty_speeds && return 0 ;;

> diff --git a/Completion/Unix/Type/_tty_speeds b/Completion/Unix/Type/_tty_speeds
> new file mode 100644
> index 0000000..ac82d60
> --- /dev/null
> +++ b/Completion/Unix/Type/_tty_speeds
> @@ -0,0 +1,7 @@
> +#autoload
> +
> +local expl
> +
> +_description 'tty speed' expl 'tty speed'

The first 'tty speed' argument is the tag.  Conventionally, tag names
don't contain spaces.  (You can type <C-X><h> to view tag names.)  The
tag name can be 'values' or perhaps 'tty-speeds'.  The incumbent code
uses 'values', at least in one place.

The second 'tty speed' argument is the description.  Since I set «zstyle
':completion:*:descriptions' format '> %d'», I expect that argument to
be displayed as a "> tty speed" line above the suggested completions
when I do «cu -s <TAB>»; however, there is no such line.

It seems you don't pass "$expl[@]" to compadd, contrary to the example
in the docs of _description.

Or perhaps the 'compadd' should be wrapped by a _wanted, as in
   _wanted values expl 'tty speed' compadd -- 75 110 ...

> +compadd -- 75 110 300 1200 2400 4800 9600 19200 38400 57600 115200
> 

I suppose the 230400 from _gdb should be added here, too?

> diff --git a/Completion/Unix/Type/_tty_lines b/Completion/Unix/Type/_tty_lines
> new file mode 100644
> index 0000000..2d9d097
> --- /dev/null
> +++ b/Completion/Unix/Type/_tty_lines
> @@ -0,0 +1,7 @@
> +#autoload
> +
> +local expl
> +
> +_description files expl 'tty line'
> +
> +_files -g '/dev/(cuaU#<->|ttyS<->)(N%c)'

Here, too, the 'tty line' description isn't shown to the user.

«cu -l /dev/<TAB>» offers only immediate subdirectories of /dev, but
doesn't offer the line device names, even though «cu -l <TAB>» does.
I suspect _files assumes the pattern doesn't start with a slash.
(This issue particularly affects _screen: that function won't call
into _tty_lines until PREFIX=/dev/, so your patch effectively breaks
«screen /dev/<TAB>».)

«cu -l <TAB>» offers both /dev/ttyS<-> and immediate subdirectories of
cwd; the latter is probably unnecessary.

When cwd is /dev, «cu -l tty<TAB>» offers all tty devices, not only
those whose names match ttyS<->.

Cheers,

Daniel