Summary

Add Bash and Zsh auto-completions.

Review Request #7668 — Created Sept. 30, 2015 and submitted Oct. 28, 2015, 1:27 a.m.

Information

Owner

jwu

Repository

RBTools

Branch

master

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

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.

Testing Done

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.

Issues

Description	From	Last Updated
Can we name this rbt-completion-setup.py ?	brennie	Oct. 2, 2015, 8:13 a.m.
Why are we including this and tools/rbt-bash-completion? They look very similar... If they should be the same, can we just …	brennie	Oct. 2, 2015, 8:13 a.m.
Likewise, this looks very similar to contrib/toolsrbt-zsh-completion.	brennie	Oct. 2, 2015, 8:13 a.m.
This should use a with statement, e.g.: try: with open(location, 'w') as f: f.write('...') except IOError as e: print('ERR ...') …	brennie	Oct. 1, 2015, 12:57 p.m.
One thing we could do here would would be super neat is to auto-detect what the login shell is. The …	brennie	Oct. 1, 2015, 1:48 p.m.
This should be: Usage: rbt-completion-setup <shell>	brennie	Oct. 1, 2015, 1:35 p.m.
<option>	brennie	Oct. 1, 2015, 1:35 p.m.
Trailing whitespace.	brennie	Oct. 1, 2015, 1:13 p.m.
This should probably say bash not <option>.	brennie	Oct. 1, 2015, 2:56 p.m.
lowercase bash	brennie	Oct. 1, 2015, 1:30 p.m.
Trailing whitespace.	brennie	Oct. 1, 2015, 1:30 p.m.
lowercase zsh	brennie	Oct. 1, 2015, 1:30 p.m.
lowercase bash.	brennie	Oct. 1, 2015, 1:30 p.m.
lowercase zsh	brennie	Oct. 1, 2015, 1:30 p.m.
You should mention you can install for bash without using sudo.	brennie	Oct. 2, 2015, 1:33 p.m.
Why does this file begin with an underscore?	brennie	Oct. 2, 2015, 1:37 p.m.
``<shell>``	brennie	Oct. 4, 2015, 12:25 p.m.
:cmd:`sudo`	brennie	Oct. 4, 2015, 12:26 p.m.
:cmd:`sudo`	brennie	Oct. 4, 2015, 12:26 p.m.
Instead of using this as an option, can we just use the args provided to main ?	brennie	Oct. 4, 2015, 3:37 p.m.
This should take shell=None as an argument so we don't have to use a flag.	brennie	Oct. 4, 2015, 3:37 p.m.
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' …	brennie	Oct. 4, 2015, 3:37 p.m.
Given the above comments, this can just compare against shell.	brennie	Oct. 4, 2015, 3:37 p.m.
We should mention the shell they give is unsupported.	brennie	Oct. 4, 2015, 1:39 p.m.
'Option' imported but unused	reviewbot	Oct. 4, 2015, 3:45 p.m.
These code paths don't ensure that /etc/bash_completion.d/ exists.	brennie	Oct. 5, 2015, 6:12 a.m.
I feel like we should have a map of: SHELLS = { 'bash': { 'Linux': { 'src': 'conf/rbt-bash-completion', 'dest': '/etc/bash_completion.d', …	brennie	Oct. 5, 2015, 6:12 a.m.
Blank line between statement and block	brennie	Oct. 5, 2015, 6:09 a.m.
We should probably check if no shell was found and report that case to the user, too.	brennie	Oct. 5, 2015, 6:11 a.m.
Blank line between these.	brennie	Oct. 5, 2015, 5:09 p.m.
bash or zsh.	brennie	Oct. 5, 2015, 5:09 p.m.
Please give this a doc-comment, e.g. #: This comment will appear in documetation SHELLS = { # ... }	brennie	Oct. 5, 2015, 6:02 p.m.
Another note: it would be better if all FS paths were built with os.path.join.	brennie	Oct. 5, 2015, 6:36 p.m.
os.path.join(self.SHELLS[shell][system]['dest'], '_rbt')	brennie	Oct. 5, 2015, 6:04 p.m.
here too	brennie	Oct. 5, 2015, 6:05 p.m.
This will print as "... directory.Refer ..."	brennie	Oct. 5, 2015, 6:12 p.m.
This should be os.path.join(os.sep, 'etc', 'rbt-bash-completion') Likewise for all other paths.	brennie	Oct. 5, 2015, 8:35 p.m.
Col: 68 W291 trailing whitespace	reviewbot	Oct. 5, 2015, 8:53 p.m.
Can this be two print statements?	brennie	Oct. 6, 2015, 4:46 a.m.
Can we print twice and sys.exit(1)	brennie	Oct. 6, 2015, 4:47 a.m.
Can this be two individual print statements?	brennie	Oct. 6, 2015, 4:47 a.m.
--shell does not appear to be a defined option in completetion_setup.py. This also appears two more times below.	gmyers	Oct. 7, 2015, 5:34 a.m.
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/). …	gmyers	Oct. 7, 2015, 5:54 a.m.
Typo -- terminal.	gmyers	Oct. 7, 2015, 5:54 a.m.
Add period at end of string.	gmyers	Oct. 7, 2015, 5:55 a.m.
One issue I ran into is that this will fail with "bash: rbt: command not found..." if rbt is not …	gmyers	Oct. 7, 2015, 6:20 a.m.
Each shell contains paths to their -> Each shell contains paths to its	CH chronicleyu	Oct. 8, 2015, 4 p.m.
:ref:`rbt completion-setup <rbt-completion-setup>`	brennie	Oct. 13, 2015, 6:04 p.m.
space between rbt-completion-setup and <rbt-completion-setup>	brennie	Oct. 15, 2015, 2:22 p.m.
To install, run::	brennie	Oct. 13, 2015, 5:38 p.m.
``sudo``	brennie	Oct. 13, 2015, 5:38 p.m.
To install, run::	brennie	Oct. 13, 2015, 5:38 p.m.
"the following command"	brennie	Oct. 13, 2015, 5:40 p.m.
This should begin with from __future__ import unicode_literals	brennie	Oct. 13, 2015, 5:43 p.m.
This should be formatted as: #: Single line summary. #: #: Multi-line description.	brennie	Oct. 13, 2015, 5:44 p.m.
The filename can go in dest for all the shells. Its one less call to os.path.join.	brennie	Oct. 15, 2015, 2:24 p.m.
The shell-specific bits should be in the SHELLS dict above.	brennie	Oct. 13, 2015, 5:53 p.m.
this should probably use logging.error.	brennie	Oct. 15, 2015, 2:31 p.m.
Instead of saying enter a specific shell we should tell the user to re-run the program with a shell as …	brennie	Oct. 13, 2015, 5:54 p.m.
This will go into 0.8.	chipx86	Oct. 17, 2015, 7:10 p.m.
Two blank lines between sections.	chipx86	Oct. 17, 2015, 7:10 p.m.
Two blank lines between sections.	chipx86	Oct. 17, 2015, 7:10 p.m.
Two blank lines between sections.	chipx86	Oct. 17, 2015, 7:10 p.m.
Two blank lines between sections.	chipx86	Oct. 17, 2015, 7:11 p.m.
Blank line between these.	chipx86	Oct. 17, 2015, 7:11 p.m.
Just need to import os. os.path comes along for the ride.	chipx86	Oct. 17, 2015, 7:11 p.m.
This should offer a more detailed explanation of what this command does. Pretend these are docs. (They someday might be.)	chipx86	Oct. 17, 2015, 7:20 p.m.
os.path.join is great for cross-platform code, but we know the environments these are going to be in, since we're keying …	chipx86	Oct. 17, 2015, 8 p.m.
Missing an "Args" section.	chipx86	Oct. 17, 2015, 8:02 p.m.
You're not going to be able to reliably do this and read from these files. Depending on how RBTools is …	chipx86	Oct. 17, 2015, 7:59 p.m.
Missing "Args".	chipx86	Oct. 17, 2015, 8:03 p.m.
These should be a logging.error.	chipx86	Oct. 17, 2015, 8:12 p.m.
logging.error.	chipx86	Oct. 17, 2015, 8:12 p.m.
logging.error.	chipx86	Oct. 17, 2015, 8:16 p.m.
logging.error.	chipx86	Oct. 17, 2015, 8:16 p.m.
How about setup-completion? Better matches setup-repo.	chipx86	Oct. 17, 2015, 8:36 p.m.
"OS X"	mike_conley	Oct. 27, 2015, 10:06 p.m.
"OS X"	mike_conley	Oct. 27, 2015, 10:06 p.m.
Out of curiosity (and maybe this was answered earlier in the reviews and I missed it), why is this called …	mike_conley	Oct. 27, 2015, 9:55 p.m.
Typo: "currentlly" -> "currently"	mike_conley	Oct. 27, 2015, 10:08 p.m.

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

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

The issue has been resolved. Show all issues

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

jwu Oct. 1, 2015, 1:52 p.m.

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.

brennie Oct. 1, 2015, 2:35 p.m.

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

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

The issue has been resolved. Show all issues

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?

jwu Oct. 1, 2015, 1:52 p.m.

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.

brennie Oct. 1, 2015, 2:35 p.m.

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

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

The issue has been resolved. Show all issues

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

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

The issue has been resolved. Show all issues

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.

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

The issue has been resolved. Show all issues

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

jwu Oct. 1, 2015, 1:52 p.m.

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.

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

The issue has been resolved. Show all issues

This should be:

Usage:
  rbt-completion-setup <shell>

docs/rbtools/rbt/configuration/completion.rst (Diff revision 1)
The issue has been resolved. Show all issues
```
<option>
```
docs/rbtools/rbt/configuration/completion.rst (Diff revision 1)
The issue has been resolved. Show all issues
```
Trailing whitespace.
```

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.

jwu Oct. 1, 2015, 1:52 p.m.

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.

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

The issue has been resolved. Show all issues

This should probably say bash not <option>.

jwu Oct. 1, 2015, 1:52 p.m.

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

brennie Oct. 1, 2015, 2:35 p.m.

Ah I see, I misread this :)

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

docs/rbtools/rbt/configuration/completion.rst (Diff revision 1)
The issue has been resolved. Show all issues
```
lowercase bash
```
docs/rbtools/rbt/configuration/completion.rst (Diff revision 1)
The issue has been resolved. Show all issues
```
Trailing whitespace.
```
docs/rbtools/rbt/configuration/completion.rst (Diff revision 1)
The issue has been resolved. Show all issues
```
lowercase zsh
```
docs/rbtools/rbt/configuration/completion.rst (Diff revision 1)
The issue has been resolved. Show all issues
```
lowercase bash.
```
docs/rbtools/rbt/configuration/completion.rst (Diff revision 1)
The issue has been resolved. Show all issues
```
lowercase zsh
```

Diff:

Revision 2 (+224 -1)

	setup.py
	contrib/tools/_rbt-zsh-completion
	contrib/tools/rbt-bash-completion
	contrib/tools/rbt-completion-setup
	docs/rbtools/index.rst
	docs/rbtools/rbt/configuration/completion.rst
	docs/rbtools/rbt/configuration/index.rst

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

~		The command `rbt-completion-setup` is available to the user after they install rbt. This allows them 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.
	~	The command `rbt completion-setup` 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.

	MANIFEST.in
	setup.py
	docs/rbtools/index.rst
	docs/rbtools/rbt/configuration/completion.rst
	docs/rbtools/rbt/configuration/index.rst
	rbtools/commands/completion_setup.py
	rbtools/commands/conf/_rbt-zsh-completion
	rbtools/commands/conf/rbt-bash-completion