Merge lp:~bcsaller/pyjuju/bash-completion into lp:pyjuju

[Dustin Kirkland ]

I had a similar but different implementation of bash completion for Ensemble in lp:~kirkland/ensemble/bash-completion.

I'm deferring my branch in favor of Ben's, which is a bit better about supporting positional arguments. I have tested Ben's code and it works well for me. Could we get this merged into trunk so that Ensemble users can benefit from bash tab completion?

Thanks,
Dustin

[Gustavo Niemeyer (niemeyer)]

I'm not familiar with that shell syntax, so might be nice to have a second review
from someone that actually does. Here are a few points:

[1]

=== added directory 'etc'

Can we please name this as "misc" to avoid binding the content to /etc?

[2]

+ if [[ $cur != -* ]] && [[ $COMP_CWORD -gt 1 ]]; then

Isn't the normal form [[ A -a B ]] instead of [[ A ]] && [[ B ]] ?

[3]

+ globalOpts=( -h --verbose -v --log-file)
(...)
+ cmdOpts=( --environment --repository )
(...)
+ cmdOpts=( --dry-run -n --environment --repository)
(...)
+ cmdOpts=(--help -h)

Spacing is very uneven. All of these entries should either
have a space on both ends, or in none.

[4]

82 + status)
83 + cmdOpts=( --output --format --environment)
84 + case $curOpt in
85 + --format) optEnums=( json yaml png svg dot )
86 + esac
87 + ;;
88 + upgrade-formula)
89 + cmdOpts=( --dry-run -n --environment --repository)
90 + ;;
91 + *)
92 + cmdOpts=(--help -h)
93 + ;;

Indentation seems broken, probably due to the mix of spaces and tabs. Tabs are probably
better as they allow the reader to choose, but they should be taken as 8 spaces in that
case. 4-space tabs are likely going to break most people's expectations (and they break
Launchpad's.. see the diff below).

[5]

84 + case $curOpt in
85 + --format) optEnums=( json yaml png svg dot )
86 + esac

case is unclosed (;;).

review: Approve

[dann frazier (dannf)]

I'd suggest quoting around $cur and $curOpt references to be more liberal wrt user input.

review: Approve

[lp:~bcsaller/pyjuju/bash-completion]

258. By Benjamin Saller on 2011-07-25

review changes, spacing, indentation, quoting

[Benjamin Saller (bcsaller)]

While I think this code is an important useability tool I don't think
the implementation needs much time or focus. It was a stop gap till we
can generate the bash script from the parser automatically in a way
similar to how `bzr` does it.

That said I made minor cleanup where they were simple and when I
encountered issues I moved on. comments below.

On Wed, Jul 20, 2011 at 7:18 AM, Gustavo Niemeyer <email address hidden> wrote:
> Review: Approve
> I'm not familiar with that shell syntax, so might be nice to have a second review
> from someone that actually does.  Here are a few points:
>
> [1]
>
> === added directory 'etc'
>
> Can we please name this as "misc" to avoid binding the content to /etc?
>

Called misc in the source tree. debian/install still maps it into
/etc/bash_completion.d/

> [2]
>
> +       if [[ $cur != -* ]] && [[ $COMP_CWORD -gt 1 ]]; then
>
> Isn't the normal form [[ A -a B ]] instead of [[ A ]] && [[ B ]] ?
>

I didn't arrive at a form that work with complex expressions. So I
ended up leaving this as is.

> [3]
>
> +       globalOpts=( -h --verbose -v --log-file)
> (...)
> +               cmdOpts=( --environment --repository )
> (...)
> +            cmdOpts=( --dry-run -n --environment --repository)
> (...)
> +               cmdOpts=(--help -h)
>
> Spacing is very uneven.  All of these entries should either
> have a space on both ends, or in none.
>
> [4]
>
> 82      +    status)
> 83      +        cmdOpts=( --output --format --environment)
> 84      +        case $curOpt in
> 85      +            --format) optEnums=( json yaml png svg dot )
> 86      +        esac
> 87      +        ;;
> 88      +    upgrade-formula)
> 89      +            cmdOpts=( --dry-run -n --environment --repository)
> 90      +        ;;
> 91      +       *)
> 92      +               cmdOpts=(--help -h)
> 93      +               ;;
>
> Indentation seems broken, probably due to the mix of spaces and tabs.  Tabs are probably
> better as they allow the reader to choose, but they should be taken as 8 spaces in that
> case.  4-space tabs are likely going to break most people's expectations (and they break
> Launchpad's.. see the diff below).
>

Ran indent over the file.

> [5]
>
> 84      +        case $curOpt in
> 85      +            --format) optEnums=( json yaml png svg dot )
> 86      +        esac
>
> case is unclosed (;;).
>

fixed

> --
> https://code.launchpad.net/~bcsaller/ensemble/bash-completion/+merge/68124
> You are the owner of lp:~bcsaller/ensemble/bash-completion.
>

[lp:~bcsaller/pyjuju/bash-completion]

259. By Benjamin Saller on 2011-07-26

white space changes

=== added file 'debian/install'
--- debian/install	1970-01-01 00:00:00 +0000
+++ debian/install	2011-07-26 01:33:30 +0000
@@ -0,0 +1,1 @@
+misc/bash_completion.d/ensemble /etc/bash_completion.d/

=== added directory 'misc'
=== added directory 'misc/bash_completion.d'
=== added file 'misc/bash_completion.d/ensemble'
--- misc/bash_completion.d/ensemble	1970-01-01 00:00:00 +0000
+++ misc/bash_completion.d/ensemble	2011-07-26 01:33:30 +0000
@@ -0,0 +1,101 @@
+shopt -s progcomp
+_ensemble () {
+    local cur cmds cmdIdx cmd cmdOpts fixedWords i globalOpts
+    local curOpt optEnums
+    local IFS=$' \n'
+
+    COMPREPLY=()
+    cur=${COMP_WORDS[COMP_CWORD]}
+    cmds='add-relation add-unit bootstrap debug-hooks debug-log deploy destroy-service expose open-tunnel remove-relation remove-unit resolved set shutdown ssh status terminate-machine unexpose upgrade-formula'
+    globalOpts=( -h --verbose -v --log-file)
+
+    # do ordinary expansion if we are anywhere after a -- argument
+    for ((i = 1; i < COMP_CWORD; ++i)); do
+        [[ ${COMP_WORDS[i]} == "--" ]] && return 0
+    done
+
+    # find the command; it's the first word not starting in -
+    cmd=
+    for ((cmdIdx = 1; cmdIdx < ${#COMP_WORDS[@]}; ++cmdIdx)); do
+        if [[ ${COMP_WORDS[cmdIdx]} != -* ]]; then
+            cmd=${COMP_WORDS[cmdIdx]}
+            break
+        fi
+    done
+
+    # complete command name if we are not already past the command
+    if [[ $COMP_CWORD -le cmdIdx ]]; then
+        COMPREPLY=( $( compgen -W "$cmds ${globalOpts[*]}" -- "$cur" ) )
+        return 0
+    fi
+
+    # find the option for which we want to complete a value
+    curOpt=
+    if [[ $cur != -* ]] && [[ $COMP_CWORD -gt 1 ]]; then
+        curOpt=${COMP_WORDS[COMP_CWORD - 1]}
+        if [[ "$curOpt" == = ]]; then
+            curOpt=${COMP_WORDS[COMP_CWORD - 2]}
+        elif [[ "$cur" == : ]]; then
+            cur=
+            curOpt="$curOpt:"
+        elif [[ "$curOpt" == : ]]; then
+            curOpt=${COMP_WORDS[COMP_CWORD - 2]}:
+        fi
+    fi
+
+    cmdOpts=( )
+    optEnums=( )
+    fixedWords=( )
+    case "$cmd" in
+        add-relation|add-unit|debug-hooks|destory-service|expose-service|unexpose-service|open-tunnel|remove-relation|remove-unit|ssh|terminate-machine)
+            cmdOpts=( --environment )
+            ;;
+        bootstrap)
+            cmdOpts=( )
+            ;;
+        set|deploy)
+            cmdOpts=( --environment --repository )
+            ;;
+        debug-log)
+            cmdOpts=( --environment -e --replay -r -i -x -l -n -o --output)
+            case "$curOpt" in
+                -l) optEnums=( DEBUG INFO ERROR WARNING CRITICAL ) ;;
+            esac
+            ;;
+        resolved)
+            cmdOpts=( --retry --environment )
+            ;;
+        status)
+            cmdOpts=( --output --format --environment)
+            case "$curOpt" in
+                --format) optEnums=( json yaml png svg dot ) ;;
+            esac
+            ;;
+        upgrade-formula)
+            cmdOpts=( --dry-run -n --environment --repository)
+            ;;
+        *)
+            cmdOpts=(--help -h)
+            ;;
+    esac
+
+    IFS=$'\n'
+    if [[ "$cur" == = ]] && [[ ${#optEnums[@]} -gt 0 ]]; then
+        # complete directly after "--option=", list all enum values
+        COMPREPLY=( "${optEnums[@]}" )
+        return 0
+    else
+        fixedWords=( "${cmdOpts[@]}"
+                     "${globalOpts[@]}"
+                     "${optEnums[@]}"
+                     "${fixedWords[@]}" )
+    fi
+
+    if [[ ${#fixedWords[@]} -gt 0 ]]; then
+        COMPREPLY=( $( compgen -W "${fixedWords[*]}" -- "$cur" ) )
+    fi
+
+    return 0
+}
+
+complete -F _ensemble -o default ensemble