Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm
Precedence: bulk
Received-SPF: pass (ns1.primenet.com.au: SPF record at _spf.mail.yahoo.com designates 212.82.96.59 as permitted sender)
cc: zsh-workers@zsh.org
In-reply-to: <20160731110516.GA20521@pileus.org>
From: Oliver Kiddle <okiddle@yahoo.co.uk>
References: <20160731110516.GA20521@pileus.org>
To: Andy Spencer <andy@pileus.org>
Subject: Re: [PATCH] Add completion for cpupower command
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <16721.1470082775.1@hydra.kiddle.eu>
Date: Mon, 01 Aug 2016 22:19:35 +0200
Message-ID: <16722.1470082775@hydra.kiddle.eu>

Andy Spencer wrote:
> These are mostly static options with the exception of the frequency-set
> arguments for the governor and min/max frequencies, which are pulled
> from the output of frequency-info.

Thanks for contributing this. I've taken a look and have a few comments.
Mostly all minor points.

> +#compdef cpupower
> +
> +local -a ret state cmds args expl context line

ret should be initialised to 1 and shouldn't be of array type.

In this function, I'd recommand having _arguments update $curcontext
instead of using the context array. So pass -C to _arguments and use:
  local curcontext="$curcontext"

The array form with $context is only necessary if more than one state
is simultaneously possible. That happens when you have optional
arguments which cpupower doesn't.

> +
> +_arguments \
> +  '(-h --help)'{-h,--help}'[print usage information]' \
> +  '(-v --version)'{-v,--version}'[print package name and version]' \

As is often the case with version and help options, no other options are
valid following them. So these two exclusion lists can be reduced to
(- :)
e.g:
  '(- :)'{-h,--help}'[print usage information]'

Also, doesn't cpupower also have a -c/--cpu option for selecting
specific CPUs and -d/--debug?

> +  ':cmd:->cmds' \
> +  '*::arg:->args' && ret=0
> +
> +case $state in
> +  cmds)

It'd also be useful to complete these commands after the help command
but this doesn't do that.

> +  args)
> +    args=( )
> +    case ${words[1]} in

With commands that take subcommands, it is common practice to insert the
subcommand in to the command component of $curcontext at this point.
Something like:
  curcontext="${curcontext%:*}-$words[1]:"

> +      frequency-info)
> +        args+=(
> +          '(-e --debug)'{-e,--debug}'[print debug info]'
> +          '(-f --freq)'{-f,--freq}'[show current frequency]'
> +          '(-w --hwfreq)'{-w,--hwfreq}'[show current hardware frequency]'
> +          '(-l --hwlimits)'{-l,--hwlimits}'[show min/max frequency allowed]'
> +          '(-d --driver)'{-d,--driver}'[show the kernel driver in use]'
> +          '(-p --policy)'{-p,--policy}'[show the current cpufreq policy]'
> +          '(-g --governors)'{-g,--governors}'[show available governers]'
> +          '(-r --related-cpus)'{-a,--related-cpus}'[show cpus that run at the same frequency]'
> +          '(-a --affected-cpus)'{-a,--affected-cpus}'[show software controlled cpus]'
> +          '(-s --stats)'{-s,--stats}'[show cpufreq statistics]'
> +          '(-y --latency)'{-y,--latency}'[show frequency change latency]'
> +          '(-o --proc)'{-o,--proc}'[print old style proc info]'
> +          '(-m --human)'{-m,--human}'[use human readable output]'
> +          '(-n --no-rounding)'{-n,--no-rounding}'[disable rounding of values]'
> +        )

The exclusion lists could be a lot more restrictive. Most combinations
print:
  You can't specify more than one --cpu parameter and/or
  more than one output-specific argument
I think it is perhaps only the last three options that can be used with others.

> +      frequency-set)

> +          '(-f --freq)'{-f,--freq}'[new frequency for userspace governor]:freq->freq'

There's a colon missing on that line.


> +        args+=(
> +          '(-d --disable)'{-d,--disable}'[disable specific sleep state]:stateno'
> +          '(-e --enable)'{-e,--enable}'[enable specific sleep state]:stateno'

Not especially important but it's not possible to complete those
states is it? Documentation points to
/sys/devices/system/cpu/cpu*/cpuidle/state*. Those don't exist on
my system but that might be because it's very old.

> +      monitor)

> +          '-i[mesurement interval]:secs'

measurement

> +    _arguments "${args[@]}" && ret=0

You can pass -s to this _arguments as it seems cpupower lets you clump
together options, e.g. cpupower frequency-info -fn

> +    case $state in
> +      freq)
> +	compadd $(cpupower frequency-info |
> +		sed -n 's/ //g; s/,/ /g; s/availablefrequencysteps://p')

It'd be nice to have descriptions for this and the governors. e.g.:
  _wanted frequencies expl frequency compadd ....
And if you stick to the context array and no -C option to _arguments,
this would need to be:
   _wanted -C "$context" frequencies ...

Oliver