Code review comment for lp:~bcsaller/pyjuju/bash-completion

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.
>