Re: [patch] Completion for _deborphan and _xrandr

[Oliver Kiddle <okiddle@yahoo.co.uk>]

gi1242+zsh@gmail.com wrote:
>
> I wrote a rudimentary completion script for deborphan (which probably
> belongs with the Debian completion commands).

Thanks for this.

> I also fixed the xrandr completion to separate outputs into two groups
> (connected and disconnected). Patch, and whole file attached. If there

Is it useful to complete a disconnected output then? If it's only
relevant with some options then we could have separate states for
completing all vs. only connected outputs.

> is a preferred way to submit this (e.g. by forking a git repository) let
> me know and I would be happy to do so.

The preferred way to submit this is by posting a patch, exactly as you
have done.

I've got a couple of minor comments and suggestions on the function:

> +    _alternative \
> +	'connected:connected outputs:('${(j: :)${(uo)${(M)xrandr_output:#* connected*}%% *}}')' \
> +	'disconnected:disconnected outputs:('${(j: :)${(uo)${(M)xrandr_output:#* disconnected*}%% *}}')' \
> +	&& return 0

The normal convention is for the group descriptions to be singular: they
describe what comes on the command line rather than the matches
collectively. So, "connected output" rather than "connected outputs".

> local keep=/var/lib/deborphan/keep
> _arguments : \
>   {--help,-h}'[help]' \
>   {--status-file,-f}'[statusfile]:file:_files' \

The description here is rather terse.

>   {--exclude,-e}'[work as if packages in LIST were not installed]:LIST:' \

I'm guessing that LIST here has come from the --help output. The
description's inclusion of "LIST" makes less sense without corresponding
text saying something like [ --exclude LIST ]. The word "specify" is
often used when rewording these. Also, is it not perhaps valid to
complete a list of installed packages here. So:

  {--exclude,-e}'[work as if specified packages were not installed]:package:_sequence _deb_packages - installed' \

That is assuming a comma-separated list. If deborphan allows you to pass
  -e pkg1 -e pkg2
Then you need to prefix the _arguments spec with a *:
  \*{--exclude,-e}'[....

>   {--force-hold,-H}'[Ignore hold flags.]' \

This description is inconsistent with the earlier ones. Our normal
convention is to neither capitalise the first word nor finish with a
full stop.

>   {--priority,-p}'[PRIOR  Select only packages with priority >= PRIOR.]:PRIOR:' \

The same comment I made about LIST applies here and in some later
places. Is there a default priority. It can be good to provide hints,
perhaps:
   :priority (1-100) [50]'
I'm assuming the priorities are just numbers.

>   {--del-keep,-R}"[PKGS.. Remove PKGS from the 'keep' file.]:*:package:_values package $(< $keep)" \

Is _values really needed here or would compadd do the job?

I'd also be inclined to try to avoid it dumping error messages to the
terminal if the $keep file doesn't exist. Perhaps:
  keep=( /var/lib/deborphan/keep(N) /dev/null)
  ..
  compadd $(<$keep[1]})

Thanks again.

Oliver