Add Bash and Zsh auto-completions.

Review Request #7668 — Created Sept. 30, 2015 and submitted

jwu
RBTools
master
rbtools

Bash and Zsh are Unix shells that have auto-completion features.

The command rbt setup-completion allows the user to install completions for Zsh or Bash via the command. However, as Bash and Zsh are in protected directories that the user cannot write to, sudo is needed to install the scripts. A manual installation method is also available in the documentation.

Ran unit tests for rbt.

Successfully installed completion for Bash and Zsh on OSX.

Successfully installed completion for Bash and Zsh on Ubuntu.

Successfully built documentation.

Description From Last Updated

Can we name this rbt-completion-setup.py ?

brenniebrennie

Why are we including this and tools/rbt-bash-completion? They look very similar... If they should be the same, can we just ...

brenniebrennie

Likewise, this looks very similar to contrib/toolsrbt-zsh-completion.

brenniebrennie

This should use a with statement, e.g.: try: with open(location, 'w') as f: f.write('...') except IOError as e: print('ERR ...') ...

brenniebrennie

One thing we could do here would would be super neat is to auto-detect what the login shell is. The ...

brenniebrennie

This should be: Usage: rbt-completion-setup <shell>

brenniebrennie

<option>

brenniebrennie

Trailing whitespace.

brenniebrennie

This should probably say bash not <option>.

brenniebrennie

lowercase bash

brenniebrennie

Trailing whitespace.

brenniebrennie

lowercase zsh

brenniebrennie

lowercase bash.

brenniebrennie

lowercase zsh

brenniebrennie

You should mention you can install for bash without using sudo.

brenniebrennie

Why does this file begin with an underscore?

brenniebrennie

``<shell>``

brenniebrennie

:cmd:`sudo`

brenniebrennie

:cmd:`sudo`

brenniebrennie

Instead of using this as an option, can we just use the args provided to main ?

brenniebrennie

This should take shell=None as an argument so we don't have to use a flag.

brenniebrennie

You should fetch the shell with: login_shell = os.environ.get(b'SHELL') if shell: login_shell = os.path.basename(shell) We are not guaranteed that 'SHELL' ...

brenniebrennie

Given the above comments, this can just compare against shell.

brenniebrennie

We should mention the shell they give is unsupported.

brenniebrennie

'Option' imported but unused

reviewbotreviewbot

These code paths don't ensure that /etc/bash_completion.d/ exists.

brenniebrennie

I feel like we should have a map of: SHELLS = { 'bash': { 'Linux': { 'src': 'conf/rbt-bash-completion', 'dest': '/etc/bash_completion.d', ...

brenniebrennie

Blank line between statement and block

brenniebrennie

We should probably check if no shell was found and report that case to the user, too.

brenniebrennie

Blank line between these.

brenniebrennie

bash or zsh.

brenniebrennie

Please give this a doc-comment, e.g. #: This comment will appear in documetation SHELLS = { # ... }

brenniebrennie

Another note: it would be better if all FS paths were built with os.path.join.

brenniebrennie

os.path.join(self.SHELLS[shell][system]['dest'], '_rbt')

brenniebrennie

here too

brenniebrennie

This will print as "... directory.Refer ..."

brenniebrennie

This should be os.path.join(os.sep, 'etc', 'rbt-bash-completion') Likewise for all other paths.

brenniebrennie

Col: 68 W291 trailing whitespace

reviewbotreviewbot

Can this be two print statements?

brenniebrennie

Can we print twice and sys.exit(1)

brenniebrennie

Can this be two individual print statements?

brenniebrennie

--shell does not appear to be a defined option in completetion_setup.py. This also appears two more times below.

gmyersgmyers

For bash on CentOS 7.0 and Fedora 15 boxes I do not have an /etc/bash_completion (but there is an /etc/bash_completion.d/). ...

gmyersgmyers

Typo -- terminal.

gmyersgmyers

Add period at end of string.

gmyersgmyers

One issue I ran into is that this will fail with "bash: rbt: command not found..." if rbt is not ...

gmyersgmyers

Each shell contains paths to their -> Each shell contains paths to its

CH chronicleyu

:ref:`rbt completion-setup <rbt-completion-setup>`

brenniebrennie

space between rbt-completion-setup and <rbt-completion-setup>

brenniebrennie

To install, run::

brenniebrennie

``sudo``

brenniebrennie

To install, run::

brenniebrennie

"the following command"

brenniebrennie

This should begin with from __future__ import unicode_literals

brenniebrennie

This should be formatted as: #: Single line summary. #: #: Multi-line description.

brenniebrennie

The filename can go in dest for all the shells. Its one less call to os.path.join.

brenniebrennie

The shell-specific bits should be in the SHELLS dict above.

brenniebrennie

this should probably use logging.error.

brenniebrennie

Instead of saying enter a specific shell we should tell the user to re-run the program with a shell as ...

brenniebrennie

This will go into 0.8.

chipx86chipx86

Two blank lines between sections.

chipx86chipx86

Two blank lines between sections.

chipx86chipx86

Two blank lines between sections.

chipx86chipx86

Two blank lines between sections.

chipx86chipx86

Blank line between these.

chipx86chipx86

Just need to import os. os.path comes along for the ride.

chipx86chipx86

This should offer a more detailed explanation of what this command does. Pretend these are docs. (They someday might be.)

chipx86chipx86

os.path.join is great for cross-platform code, but we know the environments these are going to be in, since we're keying ...

chipx86chipx86

Missing an "Args" section.

chipx86chipx86

You're not going to be able to reliably do this and read from these files. Depending on how RBTools is ...

chipx86chipx86

Missing "Args".

chipx86chipx86

These should be a logging.error.

chipx86chipx86

logging.error.

chipx86chipx86

logging.error.

chipx86chipx86

logging.error.

chipx86chipx86

How about setup-completion? Better matches setup-repo.

chipx86chipx86

"OS X"

mike_conleymike_conley

"OS X"

mike_conleymike_conley

Out of curiosity (and maybe this was answered earlier in the reviews and I missed it), why is this called ...

mike_conleymike_conley

Typo: "currentlly" -> "currently"

mike_conleymike_conley
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/configuration/index.rst
        docs/rbtools/index.rst
        docs/rbtools/rbt/configuration/completion.rst
        contrib/tools/rbt-bash-completion
        contrib/tools/rbt-completion-setup
        contrib/tools/_rbt-zsh-completion
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/configuration/index.rst
        docs/rbtools/index.rst
        docs/rbtools/rbt/configuration/completion.rst
        contrib/tools/rbt-bash-completion
        contrib/tools/rbt-completion-setup
        contrib/tools/_rbt-zsh-completion
    
    
  2. 
      
brennie
  1. 
      
  2. contrib/tools/rbt-completion-setup (Diff revision 1)
     
     

    Can we name this rbt-completion-setup.py ?

    1. If we give it a file extension, the command will appear as rbt-completion-setup.py when run in the terminal rather than just rbt-completion-setup.

      We can give it a .py extension if we include it as a console_scripts entry point and move the file to somewhere like rbtools/utils.

    2. I don't see an issue with having it names rbt-completion-setup.py.

  3. contrib/tools/rbt-completion-setup (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Why are we including this and tools/rbt-bash-completion? They look very similar...

    If they should be the same, can we just package those two files and read them in this script?

    1. I included tools/rbt-bash-completion and tools/_rbt-zsh-completion so the user could easily download the file they needed rather than look through this script to find the correct bash completion or zsh completion that they need.

    2. Then we should really just be including them when we install rbtools. You'll want to edit the MANIFEST.in file to include them, I believe. Then we can then access those scripts via pkg_resources (instead of having the code twice).

  4. contrib/tools/rbt-completion-setup (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Likewise, this looks very similar to contrib/toolsrbt-zsh-completion.

  5. contrib/tools/rbt-completion-setup (Diff revision 1)
     
     
     
     
     
     
     
     

    This should use a with statement, e.g.:

    try:
        with open(location, 'w') as f:
            f.write('...')
    except IOError as e:
        print('ERR ...')
        sys.exit()
    

    This ensures that the file handle is closed even in the case of an exception.

  6. contrib/tools/rbt-completion-setup (Diff revision 1)
     
     
     
     
     
     
     
     
     
     

    One thing we could do here would would be super neat is to auto-detect what the login shell is. The $SHELL environment variable corresponds to the user's login shell (e.g., /bin/bash). We can split this up and check if the last part of the path is one of zsh or bash and do the right thing in that case.

    (FYI, the os.environ dict contains all the environment variable mappings.)

    1. Just tried this out and it worked well!

      I left in the options since a user may want to install auto-completions on the other shell.

  7. contrib/tools/rbt-completion-setup (Diff revision 1)
     
     

    This should be:

    Usage:
      rbt-completion-setup <shell>
    
  8. Trailing whitespace.

  9. docs/rbtools/rbt/configuration/completion.rst (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Can't we autodetect ~/.{bash,zsh}{rc,_profile} and stick it in the appropriate place?

    This also might be super awkward and hacky, so feel free to ignore this if its too hard.

    1. If I'm understanding correctly, I think we could, without too much difficulty, perform the manual installation section automatically.

      The current method is just because the completion directories I have right now, are the preferred ones to install completions to.

  10. This should probably say bash not <option>.

    1. It said <option> because in Linux both Bash and Zsh are installed to protected directories. So the user still had to specify the shell.

    2. Ah I see, I misread this :)

      Can we call it <shell> and not <option>?

  11. lowercase bash

  12. Trailing whitespace.

  13. lowercase zsh

  14. lowercase bash.

  15. lowercase zsh

  16. 
      
JW
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/configuration/index.rst
        docs/rbtools/index.rst
        docs/rbtools/rbt/configuration/completion.rst
        contrib/tools/rbt-bash-completion
        contrib/tools/rbt-completion-setup
        contrib/tools/_rbt-zsh-completion
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/configuration/index.rst
        docs/rbtools/index.rst
        docs/rbtools/rbt/configuration/completion.rst
        contrib/tools/rbt-bash-completion
        contrib/tools/rbt-completion-setup
        contrib/tools/_rbt-zsh-completion
    
    
  2. 
      
JW
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/index.rst
        docs/rbtools/rbt/configuration/completion.rst
        MANIFEST.in
        docs/rbtools/rbt/configuration/index.rst
        rbtools/commands/conf/rbt-bash-completion
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/index.rst
        docs/rbtools/rbt/configuration/completion.rst
        MANIFEST.in
        docs/rbtools/rbt/configuration/index.rst
        rbtools/commands/conf/rbt-bash-completion
    
    
  2. 
      
brennie
  1. 
      
  2. docs/rbtools/rbt/configuration/completion.rst (Diff revision 3)
     
     
     
     
     
     

    You should mention you can install for bash without using sudo.

  3. Why does this file begin with an underscore?

    1. It's convention for Zsh auto-completion files to begin with an underscore.

      I also tried naming the file without the underscore and the completion file would not load.

  4. 
      
JW
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/index.rst
        docs/rbtools/rbt/configuration/completion.rst
        MANIFEST.in
        docs/rbtools/rbt/configuration/index.rst
        rbtools/commands/conf/rbt-bash-completion
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/index.rst
        docs/rbtools/rbt/configuration/completion.rst
        MANIFEST.in
        docs/rbtools/rbt/configuration/index.rst
        rbtools/commands/conf/rbt-bash-completion
    
    
  2. 
      
brennie
  1. 
      
    1. Nevermind, it should just be `sudo`. I thought :cmd: was a thing

  2. rbtools/commands/completion_setup.py (Diff revision 4)
     
     
     
     
     
     
     
     
     

    Instead of using this as an option, can we just use the args provided to main ?

    1. So, I just tried this and if you run the command as rbt completion-setup bash, the variable will be detected, but if you run sudo rbt completion-setup bash you get the following:

      usage: rbt completion-setup [options] 
      rbt: error: Invalid number of arguments provided
      

      I thought it might have something to do with run_from_argv() in the Command class. Strangely enough, if I edited the parser.error('Invalid number of arguments provided') string to something else, say "test", and then ran the command rbt completion-setup bash random I would get the expected rbt: error: test but if I ran sudo rbt completion-setup bash random I would get rbt: error: Invalid number of arguments provided. So now I'm a bit lost. Any ideas?

  3. rbtools/commands/completion_setup.py (Diff revision 4)
     
     

    This should take shell=None as an argument so we don't have to use a flag.

  4. rbtools/commands/completion_setup.py (Diff revision 4)
     
     

    You should fetch the shell with:

    login_shell = os.environ.get(b'SHELL')
    
    if shell:
        login_shell = os.path.basename(shell)
    

    We are not guaranteed that 'SHELL' is an environment variable (e.g., the user could be on Windows) and in this case .get() will return None instead of throwing a KeyError.

    Also, we should use b'ENV_VAR_NAME' because all of our literals are unicode literals and this will blow up on Windows.

    os.path.basename will give you the bash part of /usr/bin/bash but it will work in a platform-independant way.

    Finally, you only need to fetch the login shell if shell is None, e.g.:

    if not shell:
        shell = os.environ.get(b'SHELL')
        # ...
    
  5. rbtools/commands/completion_setup.py (Diff revision 4)
     
     
     
     
     
     
     

    Given the above comments, this can just compare against shell.

  6. rbtools/commands/completion_setup.py (Diff revision 4)
     
     

    We should mention the shell they give is unsupported.

  7. 
      
JW
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/index.rst
        docs/rbtools/rbt/configuration/completion.rst
        MANIFEST.in
        docs/rbtools/rbt/configuration/index.rst
        rbtools/commands/conf/rbt-bash-completion
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/index.rst
        docs/rbtools/rbt/configuration/completion.rst
        MANIFEST.in
        docs/rbtools/rbt/configuration/index.rst
        rbtools/commands/conf/rbt-bash-completion
    
    
  2. rbtools/commands/completion_setup.py (Diff revision 5)
     
     
     'Option' imported but unused
    
  3. 
      
JW
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/index.rst
        docs/rbtools/rbt/configuration/completion.rst
        MANIFEST.in
        docs/rbtools/rbt/configuration/index.rst
        rbtools/commands/conf/rbt-bash-completion
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/index.rst
        docs/rbtools/rbt/configuration/completion.rst
        MANIFEST.in
        docs/rbtools/rbt/configuration/index.rst
        rbtools/commands/conf/rbt-bash-completion
    
    
  2. 
      
brennie
  1. 
      
  2. rbtools/commands/completion_setup.py (Diff revision 6)
     
     
     
     

    These code paths don't ensure that /etc/bash_completion.d/ exists.

  3. rbtools/commands/completion_setup.py (Diff revision 6)
     
     
     

    I feel like we should have a map of:

    SHELLS = {
        'bash': {
            'Linux': {
                'src': 'conf/rbt-bash-completion',
                'dest': '/etc/bash_completion.d',
            },
            'Darwin': {
                # ...
            }
        },
        'zsh': {
            # ...
        },
    }
    

    That way, if we ever support another shell (say, fish), we can just add to that mapping instead of adding a bunch more if/else blocks.

    This will also allow the completion setup to be more succinct and can probably be made into a generic function that just looks up stuff in the mapping.

    1. Thanks for the suggestion. While implementing, I couldn't help but think how much easier this map makes things!

  4. rbtools/commands/completion_setup.py (Diff revision 6)
     
     
     

    Blank line between statement and block

  5. rbtools/commands/completion_setup.py (Diff revision 6)
     
     
     

    We should probably check if no shell was found and report that case to the user, too.

  6. 
      
JW
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/index.rst
        docs/rbtools/rbt/configuration/completion.rst
        MANIFEST.in
        docs/rbtools/rbt/configuration/index.rst
        rbtools/commands/conf/rbt-bash-completion
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/index.rst
        docs/rbtools/rbt/configuration/completion.rst
        MANIFEST.in
        docs/rbtools/rbt/configuration/index.rst
        rbtools/commands/conf/rbt-bash-completion
    
    
  2. 
      
brennie
  1. Just a few more minor issues and this is good to go :)

    If you haven't, you should build the docs and mention that in your testing done.

  2. rbtools/commands/completion_setup.py (Diff revision 7)
     
     
     

    Blank line between these.

  3. rbtools/commands/completion_setup.py (Diff revision 7)
     
     

    bash or zsh.

  4. rbtools/commands/completion_setup.py (Diff revision 7)
     
     

    Please give this a doc-comment, e.g.

    #: This comment will appear in documetation
    SHELLS = {
        # ...
    }
    
  5. rbtools/commands/completion_setup.py (Diff revision 7)
     
     

    os.path.join(self.SHELLS[shell][system]['dest'], '_rbt')

  6. rbtools/commands/completion_setup.py (Diff revision 7)
     
     

    here too

  7. rbtools/commands/completion_setup.py (Diff revision 7)
     
     
     
     

    This will print as

    "... directory.Refer ..."

  8. 
      
brennie
  1. 
      
  2. rbtools/commands/completion_setup.py (Diff revision 7)
     
     

    Another note: it would be better if all FS paths were built with os.path.join.

  3. 
      
JW
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/index.rst
        docs/rbtools/rbt/configuration/completion.rst
        MANIFEST.in
        docs/rbtools/rbt/configuration/index.rst
        rbtools/commands/conf/rbt-bash-completion
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/index.rst
        docs/rbtools/rbt/configuration/completion.rst
        MANIFEST.in
        docs/rbtools/rbt/configuration/index.rst
        rbtools/commands/conf/rbt-bash-completion
    
    
  2. 
      
brennie
  1. 
      
  2. rbtools/commands/completion_setup.py (Diff revision 8)
     
     

    This should be os.path.join(os.sep, 'etc', 'rbt-bash-completion')

    Likewise for all other paths.

  3. 
      
JW
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/index.rst
        docs/rbtools/rbt/configuration/completion.rst
        MANIFEST.in
        docs/rbtools/rbt/configuration/index.rst
        rbtools/commands/conf/rbt-bash-completion
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/index.rst
        docs/rbtools/rbt/configuration/completion.rst
        MANIFEST.in
        docs/rbtools/rbt/configuration/index.rst
        rbtools/commands/conf/rbt-bash-completion
    
    
  2. rbtools/commands/completion_setup.py (Diff revision 9)
     
     
    Col: 68
     W291 trailing whitespace
    
  3. 
      
JW
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/index.rst
        docs/rbtools/rbt/configuration/completion.rst
        MANIFEST.in
        docs/rbtools/rbt/configuration/index.rst
        rbtools/commands/conf/rbt-bash-completion
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/index.rst
        docs/rbtools/rbt/configuration/completion.rst
        MANIFEST.in
        docs/rbtools/rbt/configuration/index.rst
        rbtools/commands/conf/rbt-bash-completion
    
    
  2. 
      
brennie
  1. 
      
  2. rbtools/commands/completion_setup.py (Diff revision 10)
     
     
     

    Can this be two print statements?

  3. rbtools/commands/completion_setup.py (Diff revision 10)
     
     
     

    Can we print twice and sys.exit(1)

  4. rbtools/commands/completion_setup.py (Diff revision 10)
     
     
     
     

    Can this be two individual print statements?

  5. 
      
JW
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/index.rst
        docs/rbtools/rbt/configuration/completion.rst
        MANIFEST.in
        docs/rbtools/rbt/configuration/index.rst
        rbtools/commands/conf/rbt-bash-completion
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/index.rst
        docs/rbtools/rbt/configuration/completion.rst
        MANIFEST.in
        docs/rbtools/rbt/configuration/index.rst
        rbtools/commands/conf/rbt-bash-completion
    
    
  2. 
      
gmyers
  1. <p>I know you've got <code>docs/rbtools/rbt/configuration/completion.rst</code>, but should there also be a <code>docs/rbtools/rbt/commands/completion-setup.rst</code> to provide details about the new command in a manner that is consistent with all other commands?</p>

    1. Updated the doc page to be in commands rather than configuration (rbt completion-setup was not a command previously).

    2. I'm not sure this is exactly what you want.  The docs for existing commands all have a consistent template that they follow.  Perhaps this can be updated to follow this template, or moved back to the configuration area with a simple new page added specifically to describe the completion-setup command.  Also see setup-repo.rst for specifying the version the command was introduced in -- I suppose this will be 0.7.5.
    3. Updated completion command page to follow existing command style.

  2. --shell does not appear to be a defined option in completetion_setup.py. This also appears two more times below.

  3. rbtools/commands/completion_setup.py (Diff revision 11)
     
     
     

    For bash on CentOS 7.0 and Fedora 15 boxes I do not have an /etc/bash_completion (but there is an /etc/bash_completion.d/). This causes the existence check on line 58 to fail and exit with the "Could not locate bash completion file." error message.

    If I manually copy rbt-bash-completion to /etc/bash_completion.d/rbt then completions do work successfully, so it seems like the required existence of /etc/bash_completion is not viable for all linux distros. You've already confirmed that dest exists, so why is the check of completion_file even necessary?

    As an aside Ubuntu 12.04.5 LTS does have /etc/bash_completion, so this obviously exists for some distros.

    1. The /etc/bash_completion file needs to be sourced in the .bashrc file for any auto-completions to work in bash as far as I know. There should be a section in your .bashrc file doing this.

      I found in Ubuntu 14.04 the file is in /usr/share/bash-completion/bash_completion. Another common place for it seems to be in the home directory.

      However, now that you mention it, it's probably better for the script not to check for the completion file. Since even if it exists, if it isn't sourced in the .bashrc file then the auto-completions wouldn't work anyway.

  4. rbtools/commands/completion_setup.py (Diff revision 11)
     
     
    Typo -- terminal.
  5. rbtools/commands/completion_setup.py (Diff revision 11)
     
     
    Add period at end of string.
  6. rbtools/commands/conf/rbt-bash-completion (Diff revision 11)
     
     
     
     
     
     
     

    One issue I ran into is that this will fail with "bash: rbt: command not found..." if rbt is not on the path, which is probably unlikely but it could happen. I suppose grep could also be unknown or not on the path, but that seems even more unlikely.

    Switching to something like the snippet below (thanks to http://stackoverflow.com/a/677212, which may not necessarily be the best solution) resolves this issue:

        if hash rbt 2>/dev/null; then
            for opt in `rbt --help | grep -o "^  [A-Za-z\-]*\S"`
            do
                if [[ $opt != "command"* && $opt != "-"* ]]; then
                    opts+=" ${opt}"
                fi
            done
        fi
    

    I'm sure there would be a similar problem with zsh.

  7. 
      
JW
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/rbt-bash-completion
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/rbt/commands/completion.rst
        MANIFEST.in
        docs/rbtools/index.rst
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/rbt-bash-completion
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/rbt/commands/completion.rst
        MANIFEST.in
        docs/rbtools/index.rst
    
    
  2. 
      
CH
  1. 
      
  2. rbtools/commands/completion_setup.py (Diff revision 12)
     
     

    Each shell contains paths to their

    -> Each shell contains paths to its

  3. 
      
JW
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/rbt-bash-completion
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/rbt/commands/completion.rst
        MANIFEST.in
        docs/rbtools/index.rst
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/rbt-bash-completion
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/rbt/commands/completion.rst
        MANIFEST.in
        docs/rbtools/index.rst
    
    
  2. 
      
gmyers
  1. 
      
  2. rbtools/commands/conf/rbt-bash-completion (Diff revisions 11 - 13)
     
     
     
     
     
     
     
     
     
     
     
     
     
    Two things....
    
    1) I think my prior feedback with respect to rbt being on the path may have been misguided.  In testing with git completions, I though there were no error messages produced in the case where git was not on the path.  But, I don't know what I was doing because I can no longer replicate that and bash will in fact complain about git and egrep not being found.  Also, for what ever reason the new message you are echoing looks totally wonky on my terminal:
    
    
    $ rbt 
    be        on        path.     rbt       requires  Script    the 
    
    
    So, it might be best to just go back to what you had originally.  Sorry about that.
    
    
    2) This one I've tested more and believe it is legitimate.  Tab completion does not revert back to "normal" behavior after the rbt command is identified.  What I mean is if I type "rbt po<tab>po<tab>po"  I'll get 'rbt post post post'.  I think this will be problematic for commands like post and diff where you can explicitly include files with the -I/--include option.  For example if I would like to produce a diff for the file page.cpp, I'd like to type "rbt di<tab>-I pa<tab>" and get 'rbt diff -I page.cpp', but instead I end up with 'rbt diff -I patch'.
    1. 1) Reverted back to the old case
      
      2) Auto-completions after the first command now defaults to completing file-names and directories.
  3. 
      
JW
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/rbt-bash-completion
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/rbt/commands/completion.rst
        MANIFEST.in
        docs/rbtools/index.rst
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/rbt-bash-completion
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/rbt/commands/completion.rst
        MANIFEST.in
        docs/rbtools/index.rst
    
    
  2. 
      
gmyers
  1. Ship It!
  2. 
      
brennie
  1. A few last nitpicks, and then I think it's ready to land.

  2. docs/rbtools/index.rst (Diff revision 14)
     
     

    :ref:`rbt completion-setup <rbt-completion-setup>`
    
  3. docs/rbtools/rbt/commands/completion.rst (Diff revision 14)
     
     

    To install, run::
    
  4. docs/rbtools/rbt/commands/completion.rst (Diff revision 14)
     
     

    ``sudo``
    
  5. docs/rbtools/rbt/commands/completion.rst (Diff revision 14)
     
     

    To install, run::
    
  6. docs/rbtools/rbt/commands/completion.rst (Diff revision 14)
     
     

    "the following command"

  7. rbtools/commands/completion_setup.py (Diff revision 14)
     
     

    This should begin with

    from __future__ import unicode_literals
    
  8. rbtools/commands/completion_setup.py (Diff revision 14)
     
     
     

    This should be formatted as:

    #: Single line summary.
    #:
    #: Multi-line description.
    
  9. rbtools/commands/completion_setup.py (Diff revision 14)
     
     
     
     
     

    The shell-specific bits should be in the SHELLS dict above.

  10. rbtools/commands/completion_setup.py (Diff revision 14)
     
     
     

    Instead of saying enter a specific shell we should tell the user to re-run the program with a shell as an argument (because we don't ask for input at this point).

  11. 
      
JW
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/rbt-bash-completion
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/rbt/commands/completion.rst
        MANIFEST.in
        docs/rbtools/index.rst
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/rbt-bash-completion
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/rbt/commands/completion.rst
        MANIFEST.in
        docs/rbtools/index.rst
    
    
  2. 
      
brennie
  1. A few minor nitpicks :)

  2. docs/rbtools/index.rst (Diff revisions 14 - 15)
     
     

    space between rbt-completion-setup and <rbt-completion-setup>

  3. rbtools/commands/completion_setup.py (Diff revisions 14 - 15)
     
     

    The filename can go in dest for all the shells. Its one less call to os.path.join.

    1. I added filename since in main(), theres an if statement where we check if the completion directory exists before continuing with the setup.

  4. rbtools/commands/completion_setup.py (Diff revisions 14 - 15)
     
     

    this should probably use logging.error.

  5. 
      
JW
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/rbt-bash-completion
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/rbt/commands/completion.rst
        MANIFEST.in
        docs/rbtools/index.rst
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/completion_setup.py
        setup.py
    
    Ignored Files:
        rbtools/commands/conf/rbt-bash-completion
        rbtools/commands/conf/_rbt-zsh-completion
        docs/rbtools/rbt/commands/completion.rst
        MANIFEST.in
        docs/rbtools/index.rst
    
    
  2. 
      
chipx86
  1. 
      
  2. docs/rbtools/rbt/commands/completion.rst (Diff revision 16)
     
     

    This will go into 0.8.

  3. docs/rbtools/rbt/commands/completion.rst (Diff revision 16)
     
     
     
     

    Two blank lines between sections.

  4. docs/rbtools/rbt/commands/completion.rst (Diff revision 16)
     
     
     
     

    Two blank lines between sections.

  5. docs/rbtools/rbt/commands/completion.rst (Diff revision 16)
     
     
     
     

    Two blank lines between sections.

  6. docs/rbtools/rbt/commands/completion.rst (Diff revision 16)
     
     
     
     

    Two blank lines between sections.

  7. rbtools/commands/completion_setup.py (Diff revision 16)
     
     
     

    Blank line between these.

  8. rbtools/commands/completion_setup.py (Diff revision 16)
     
     

    Just need to import os. os.path comes along for the ride.

  9. rbtools/commands/completion_setup.py (Diff revision 16)
     
     

    This should offer a more detailed explanation of what this command does. Pretend these are docs. (They someday might be.)

  10. rbtools/commands/completion_setup.py (Diff revision 16)
     
     
     

    os.path.join is great for cross-platform code, but we know the environments these are going to be in, since we're keying off by platform. It's fine to just set absolute paths.

    For the source path, it's actually going to need to use "/" no matter what. (See below about pkg_resources for why.)

  11. rbtools/commands/completion_setup.py (Diff revision 16)
     
     

    Missing an "Args" section.

  12. rbtools/commands/completion_setup.py (Diff revision 16)
     
     
     
     
     

    You're not going to be able to reliably do this and read from these files. Depending on how RBTools is packaged on a given system, these might be stored in a zip file.

    You'll need to instead make use of pkg_resources.resource_string, which will give you the contents of the file. This will take the module name ("rbtools") and a path within it.

  13. rbtools/commands/completion_setup.py (Diff revision 16)
     
     

    Missing "Args".

  14. rbtools/commands/completion_setup.py (Diff revision 16)
     
     
     
     

    These should be a logging.error.

  15. rbtools/commands/completion_setup.py (Diff revision 16)
     
     
     

    logging.error.

  16. rbtools/commands/completion_setup.py (Diff revision 16)
     
     
     

    logging.error.

  17. rbtools/commands/completion_setup.py (Diff revision 16)
     
     

    logging.error.

  18. setup.py (Diff revision 16)
     
     

    How about setup-completion? Better matches setup-repo.

  19. 
      
JW
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/setup_completion.py
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/setup-completion.rst
        rbtools/commands/conf/rbt-bash-completion
        rbtools/commands/conf/_rbt-zsh-completion
        MANIFEST.in
        docs/rbtools/index.rst
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/setup_completion.py
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/setup-completion.rst
        rbtools/commands/conf/rbt-bash-completion
        rbtools/commands/conf/_rbt-zsh-completion
        MANIFEST.in
        docs/rbtools/index.rst
    
    
  2. 
      
JW
mike_conley
  1. 
      
  2. Out of curiosity (and maybe this was answered earlier in the reviews and I missed it), why is this called _rbt-zsh-completion, and not rbt-zsh-completion?

    1. Zsh wouldn't recognize the file as an auto-completion file unless it had the underscore at the start of the filename.

  3. rbtools/commands/setup_completion.py (Diff revision 17)
     
     

    Typo: "currentlly" -> "currently"

  4. 
      
JW
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/setup_completion.py
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/setup-completion.rst
        rbtools/commands/conf/rbt-bash-completion
        rbtools/commands/conf/_rbt-zsh-completion
        MANIFEST.in
        docs/rbtools/index.rst
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/setup_completion.py
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/setup-completion.rst
        rbtools/commands/conf/rbt-bash-completion
        rbtools/commands/conf/_rbt-zsh-completion
        MANIFEST.in
        docs/rbtools/index.rst
    
    
  2. 
      
JW
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (92557bb)
Loading...