[Bash-completion-devel] Review Request: hellanzb

David Paleino d.paleino at gmail.com
Mon Feb 7 09:20:08 UTC 2011


Hello Stefano,

On Sun, 5 Dec 2010 14:58:38 +0200, Stefano Rivera wrote:

> I just fell down the rabbit hole of writing bash completion scripts. I
> wish I knew where my Sunday morning went... :)
> 
> Here's a completion script for hellanzb. Review please?

Sorry for the need of a poke :)

> One question:
> "_filedir -d" doesn't seem interested in subdirectories, it adds a space
> as soon as there's an unambiguous completion. It doesn't do this without
> the -d. Is that really the desired behaviour?

With the current git revision, it doesn't add any space. It must've been fixed
already.

> BTW: Are there guidelines about when bash completion scripts belong in
> bash-completion and when they belong in the package they complete for? I
> couldn't find any...

Unfortunately, not. This is a grey area, and we (the team) have had different
opinions over the time.


Here's the review:

> # hellanzb(1) completion script
> # Copyright 2010, Stefano Rivera <stefano at rivera.za.net>

We don't have a particular policy about completions licensing; I believe giving
copyright to you is not a problem, as long as the license is compatible with
ours. :)

> have hellanzb && _hellanzb_opts() {
                ^^^
It's better if you do something like:

have hellanzb && {
  ... # the whole completion
} &&
complete -F ...

> 	# First extract long options then short options & rpc commands
> 	hellanzb -h 2> /dev/null \
> 		| sed -nre '/^  .*, --/ {h; s/.*(--[a-z-]+).*/\1/ p; g}' \
> 	              -e 's/^  ([a-zA-Z-]+)[, ].*/\1/ p'
> }

This catches both "remote-calls" and proper "options". You might want to split
this.
Also, we have an helper function, _parse_help(), you might give it a look. I
can't tell whether it works or not (probably not, but it's worth a try, at
least)

> [ -n "${have:-}" ] && _hellanzb() {
  ^^^^^^^^^^^^^^^^^^^^^
See the first comment :)

> 	local cur prev
> 	COMPREPLY=()
> 	cur="${COMP_WORDS[COMP_CWORD]}"
> 	prev="${COMP_WORDS[COMP_CWORD-1]}"

Use "_get_comp_words_by_ref cur prev" instead of those two lines (cur=* and
prev=*).

> 	if [[ $COMP_CWORD -eq 1 ]]; then
> 		COMPREPLY=( $(compgen -W "$(_hellanzb_opts)" -- "$cur") )
> 		return
> 	fi
> 	if [[ "$prev" = "enqueue" ]]; then
> 		_filedir '@(nzb|NZB|zip|ZIP)'
                         ^^^^^^^^^^^^^^^^^^^^
_filedir now supports uppercase versions of the given extensions automatically.
Thus a "_filedir '@(nzb|zip)'" should suffice.

>		return
>	fi
>	local dir_options file_options
>	file_options="-c --config -l --log-file -d --debug-file"
>	if echo "" $file_options | grep -Fwq -- "$prev"; then
>		_filedir
>		return
>	fi	
>	dir_options="process -p --post-process-dir"
>	if echo "" $dir_options | grep -Fwq -- "$prev"; then
>		_filedir -d
>		return
>	fi

Given you're hardcoding some options here, does it make sense to parse them
from the help output? Why not hardcoding them all? Do they really change that
often?

>	# Complete commands if we haven't had one yet
>	local i hellanzb_opts commands
>	hellanzb_opts="$(_hellanzb_opts)"
>	commands="$(echo "" $hellanzb_opts | tr ' ' '\n' | grep -v '^-')"
>	for ((i=0; i < COMP_CWORD; i++)); do
>		if echo "" $commands | grep -Fwq -- "${COMP_WORDS[i]}"; then
>			return
>		fi
>	done
>	COMPREPLY=( $(compgen -W "$hellanzb_opts" -- "$cur") )

All of this would be simpler if you parse/hardcode commands and options in two
different variables.

Also, from the help output, I see:

    hellanzb [options] [remote-call] [remote-call-options]

so I understand that position on the command line is important. You could
follow one of the other completions, which more or less all have the following
structure:

    case $prev in
        command1)
            # complete for remote-call-options for command1
            return 0
            ;;
        ...
    esac

    if [[ $cur == -* ]]; then
        COMPREPLY=( $( compgen -W '--option1 --option2 --option3' -- "$cur" ) )
        return 0
    else
        # complete commands, since we're not starting with "-"
    fi

>}
>[ -n "${have:-}" ] && complete -F _hellanzb hellanzb

See the first comment :)

Additional comments: we usually use "return 0" instead of simply "return"; and
we prefer long options over short ones. This means, if you have "-h" and
"--help", only complete "--help". 


I have missed something for sure. Please take your time to review this
completion, and don't hesitate to ask if you need help :)

Thank you for your work!

Have a nice day,
David

-- 
 . ''`.   Debian developer | http://wiki.debian.org/DavidPaleino
 : :'  : Linuxer #334216 --|-- http://www.hanskalabs.net/
 `. `'`  GPG: 1392B174 ----|---- http://deb.li/dapal
   `-   2BAB C625 4E66 E7B8 450A C3E1 E6AA 9017 1392 B174
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/bash-completion-devel/attachments/20110207/610afe02/attachment.pgp>


More information about the Bash-completion-devel mailing list