Re: PATCH Completion for _yum (contains FIXMEs)

[Oliver Kiddle <okiddle@xxxxxxxxxxx>]

Paul Seyfert wrote:
>
> Now done as you suggest. Nice indeed (and I learned what the first
> packages in 'packages:packages:...' is good for).

The normal convention is for the descriptions to be in singular form. So
that would be 'packages:package:...'

Thanks for the update. I've got a few comments to add to what Daniel
already said.

+		"repolist:Display the configured software repositories"
+		"distribution-synchronization:Synchronize installed packages to the latest available versions"
+		"distro-sync:Synchronize installed packages to the latest available versions"

Another convention is lowercase for the descriptions, so "display",
"synchronize", etc.

+# Expand next argument after 'yum check'
+_yum_check() {
+  if (( CURRENT == 2 )); then
+    local -a chkargs
+    #chkargs=("dependencies" "duplicates" "obsoletes" "provides" "all") # according to man page
+    #chkargs=("dependencies" "duplicates" "all") # according to help
+    chkargs=("dependencies" "duplicates" "provides" "all") # what works for me
+    _describe -t arguments 'check arguments' chkargs

In quite a few cases, you're using _describe to add matches without
per-match descriptions. The whole point of _describe is to make it easy
to add matches that do have per-match descriptions. For this, you can
just use _wanted with compadd which is much more light-weight:
  local expl
  _wanted arguments expl 'check argument' compadd dependencies \
      duplicates provides all

+  return 0

Completion functions should only return 0 if they added matches.
Otherwise, completion goes on and tries approximate completion and other
fallbacks you may have configured. Often you don't need a return
statement because if the last command is _wanted or _describe then its
return status will be passed on. In complex cases, start with local
ret=1, put && ret=0 after all commands that may add matches and finish
the function with return ret.

+_ids() {

It's probably unwise to define yum specific functions with generic names
like _ids and _ranges. A common convention would be _yum_ids with
_yum-check being for completion after "yum check".

+  # the last argument will be the first valid transaction ID to be suggested
+  # all other arguments are forwarded to compadd
+  #
+  # maxid is the last transaction ID known to yum
+  # minusone is the number of arguments provided to _ids minus one
+  local maxid
+  local minusone
+
+  maxid=$(yum history stats | grep Transactions | sed "s/.*: //")
+  ((minusone=$#-1))
+  compadd "${(@)@[1,$minusone]}" $(echo {$@[$#]..$maxid})

I'm getting some stderr output from yum history stats in my terminal
from this. I don't think you need the echo inside a subshell, the
brace-expansion alone should work.

+# Expand next argument after 'yum history'
+_yum_history() {
+  if (( CURRENT == 2 )); then
+    local -a historycommands
+    historycommands=('info' 'list' 'packages-list' 'packages-info' 'summary' 'addon-info' 'redo' 'undo' 'roll-back' 'new' 'sync' 'stats')
+    _describe -t commands 'yum history command' historycommands
+  fi
+  if (( CURRENT == 3 )); then

A minor point, but you can use elif here.

+    (( ${+alts[1]} )) && _alternative "$alts[@]" || \
+      _message "unknown expansion for: yum history $words[2]"

This will print the message in the case where _alternative is run but
there were no matches which I don't think is what you want. A full
if…then…else should be used.

And rather than printing a message, I think it is best to call _default
as the fallback so that the user gets filename completion. In the case
of a new subcommand being added in a new version of yum falling back to
default completion is more graceful.

Oliver