Summary

Add support to `rbtools` for command aliases.

Review Request #6491 — Created Oct. 22, 2014 and submitted Nov. 17, 2014, 2:12 a.m.

Information

Owner

brennie

Repository

RBTools

Branch

master

Bugs

Depends On

Commit

7816b0d...

Reviewers

Groups

rbtools, students

People

Description

rbtools aliases, which are defined in the .reviewboardrc file,
come in two forms: aliases for other rbtools commands and aliases
for system commands. The former allows for convenient short-hand for
rbt commands and the latter allows for more versatile and flexible
commands that utilize the shell. Aliases allow for positional variable
substition. rbtools aliases are only attempted to be executed after
attempting to find an internal command and external command (named
rbt-<commandname>) fail.

Add documentation for rbtools aliases. Add a link to the
documentation index to reference the documentation for rbtools
aliases.

Add unit tests to test the parameter substitution for rbtools
aliases.

Testing Done

Unit tests pass.

Issues

Description	From	Last Updated
'StringIO' imported but unused	reviewbot	Oct. 22, 2014, 10:18 p.m.
'logging' imported but unused	reviewbot	Oct. 22, 2014, 10:18 p.m.
'sys' imported but unused	reviewbot	Oct. 22, 2014, 10:18 p.m.
'StringIO' imported but unused	reviewbot	Oct. 22, 2014, 10:21 p.m.
There should still be two blank lines here.	david	Oct. 26, 2014, 2:31 p.m.
Alignment is off here.	david	Oct. 26, 2014, 2:34 p.m.
How about: try: return args[index] except IndexError: return ''	david	Oct. 26, 2014, 2:33 p.m.
No need for this empty line.	david	Oct. 26, 2014, 2:31 p.m.
How about: use_shell = alias.startswith('!') if use_shell: cmd = shlex.split(alias[1:]) else: cmd = [RB_MAIN] + shlex.split(alias)	david	Oct. 26, 2014, 2:32 p.m.
Two blank lines between headers. Same below.	chipx86	Oct. 27, 2014, 2:16 p.m.
Backticks are meant as references in ReST. Missing references are shown with italics, though. Did you mean quotes, or italics?	chipx86	Oct. 27, 2014, 2:47 p.m.
It'd be nice to break this up into a few smaller paragraphs. Each conditional here ("If a position variable is …	chipx86	Oct. 27, 2014, 2:20 p.m.
No blank line here.	chipx86	Oct. 27, 2014, 2:17 p.m.
During my first read, it wasn't immediately clear about how this would affect things. OSError is so general that I …	chipx86	Oct. 27, 2014, 12:58 p.m.
Should be on the same line.	chipx86	Oct. 27, 2014, 2:17 p.m.
Where is $* handled?	chipx86	Oct. 27, 2014, 2:47 p.m.
Can this mention that it'll handle argument replacement?	chipx86	Oct. 27, 2014, 2:18 p.m.
I don't think this tests rbtools.api anymore. Probably an old comment. How about just saying "rbtools.utils"?	chipx86	Oct. 27, 2014, 2:18 p.m.
'pipes' imported but unused	reviewbot	Oct. 27, 2014, 2:48 p.m.
Col: 56 E231 missing whitespace after ','	reviewbot	Oct. 27, 2014, 2:48 p.m.
Col: 1 W391 blank line at end of file	reviewbot	Oct. 27, 2014, 2:48 p.m.
This seems like it could be much cleaner as a regex.	david	Oct. 31, 2014, 11:04 a.m.
You don't need this because of the final return call.	david	Oct. 31, 2014, 11:04 a.m.
Only one blank line between functions.	justy777	Oct. 29, 2014, 5:48 p.m.
Only one blank line between functions.	justy777	Oct. 29, 2014, 5:48 p.m.
And here.	justy777	Oct. 29, 2014, 5:48 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Oct. 31, 2014, 11:16 a.m.
Col: 24 E711 comparison to None should be 'if cond is None:'	reviewbot	Oct. 31, 2014, 11:15 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Oct. 31, 2014, 11:16 a.m.
This scares me :) So easy to have really bad bugs in command line string parsing. Can we leverage shlex.split() …	chipx86	Nov. 4, 2014, 8:08 p.m.
No periods in test docstrings. Same below.	chipx86	Nov. 4, 2014, 8:08 p.m.

Tool: Pyflakes
Processed Files:
    rbtools/commands/main.py
    rbtools/utils/aliases.py

Ignored Files:
    docs/rbtools/rbt/aliases.rst
    docs/rbtools/index.rst



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/main.py
    rbtools/utils/aliases.py

Ignored Files:
    docs/rbtools/rbt/aliases.rst
    docs/rbtools/index.rst

rbtools/commands/main.py (Diff revision 1)
The issue has been dropped. Show all issues
```
 'StringIO' imported but unused
```
rbtools/utils/aliases.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'logging' imported but unused
```
rbtools/utils/aliases.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'sys' imported but unused
```

Commit:

581fbdbb4e3e2612251ec73473ad7a60f234ca42

c3dc898a02245135beed0f104c041639abff5cdc

Diff:

Revision 2 (+193 -11)

Show changes

	docs/rbtools/index.rst
	docs/rbtools/rbt/aliases.rst
	rbtools/commands/main.py
	rbtools/utils/aliases.py
	rbtools/utils/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/main.py
    rbtools/utils/tests.py
    rbtools/utils/aliases.py

Ignored Files:
    docs/rbtools/rbt/aliases.rst
    docs/rbtools/index.rst



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/main.py
    rbtools/utils/tests.py
    rbtools/utils/aliases.py

Ignored Files:
    docs/rbtools/rbt/aliases.rst
    docs/rbtools/index.rst

rbtools/commands/main.py (Diff revision 2)
The issue has been resolved. Show all issues
```
 'StringIO' imported but unused
```

Change Summary:

remove unnecessary stringio import.

Commit:

c3dc898a02245135beed0f104c041639abff5cdc

1ecc58b8f8628f0e0ae3363c3c9e5d216368f7c5

Diff:

Revision 3 (+193 -16)

Show changes

	docs/rbtools/index.rst
	docs/rbtools/rbt/aliases.rst
	rbtools/commands/main.py
	rbtools/utils/aliases.py
	rbtools/utils/tests.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/main.py
    rbtools/utils/tests.py
    rbtools/utils/aliases.py

Ignored Files:
    docs/rbtools/rbt/aliases.rst
    docs/rbtools/index.rst



Tool: Pyflakes
Processed Files:
    rbtools/commands/main.py
    rbtools/utils/tests.py
    rbtools/utils/aliases.py

Ignored Files:
    docs/rbtools/rbt/aliases.rst
    docs/rbtools/index.rst

Change Summary:

Formatting.

Commit:

1ecc58b8f8628f0e0ae3363c3c9e5d216368f7c5

e81e39ae25c76c6d51567e6cd459487e22747479

Diff:

Revision 4 (+194 -16)

Show changes

	docs/rbtools/index.rst
	docs/rbtools/rbt/aliases.rst
	rbtools/commands/main.py
	rbtools/utils/aliases.py
	rbtools/utils/tests.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/main.py
    rbtools/utils/tests.py
    rbtools/utils/aliases.py

Ignored Files:
    docs/rbtools/rbt/aliases.rst
    docs/rbtools/index.rst



Tool: Pyflakes
Processed Files:
    rbtools/commands/main.py
    rbtools/utils/tests.py
    rbtools/utils/aliases.py

Ignored Files:
    docs/rbtools/rbt/aliases.rst
    docs/rbtools/index.rst

rbtools/commands/main.py (Diff revision 4)
The issue has been resolved. Show all issues
```
There should still be two blank lines here.
```
rbtools/commands/main.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Alignment is off here.
```

rbtools/utils/aliases.py (Diff revision 4)

The issue has been resolved. Show all issues

How about:

try:
    return args[index]
except IndexError:
    return ''

rbtools/utils/aliases.py (Diff revision 4)
The issue has been resolved. Show all issues
```
No need for this empty line.
```

rbtools/utils/aliases.py (Diff revision 4)

The issue has been resolved. Show all issues

How about:

use_shell = alias.startswith('!')
if use_shell:
    cmd = shlex.split(alias[1:])
else:
    cmd = [RB_MAIN] + shlex.split(alias)

Change Summary:

Fix the issues in David's review

Commit:

e81e39ae25c76c6d51567e6cd459487e22747479

e6c473089cbc8f9138fa2dad960b8aee461bbbb7

Diff:

Revision 5 (+192 -15)

Show changes

	docs/rbtools/index.rst
	docs/rbtools/rbt/aliases.rst
	rbtools/commands/main.py
	rbtools/utils/aliases.py
	rbtools/utils/tests.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/main.py
    rbtools/utils/tests.py
    rbtools/utils/aliases.py

Ignored Files:
    docs/rbtools/rbt/aliases.rst
    docs/rbtools/index.rst



Tool: Pyflakes
Processed Files:
    rbtools/commands/main.py
    rbtools/utils/tests.py
    rbtools/utils/aliases.py

Ignored Files:
    docs/rbtools/rbt/aliases.rst
    docs/rbtools/index.rst

docs/rbtools/rbt/aliases.rst (Diff revision 5)
The issue has been resolved. Show all issues
```
Two blank lines between headers.
Same below.
```

docs/rbtools/rbt/aliases.rst (Diff revision 5)

The issue has been resolved. Show all issues

Backticks are meant as references in ReST. Missing references are shown with italics, though. Did you mean quotes, or italics?

docs/rbtools/rbt/aliases.rst (Diff revision 5)

The issue has been resolved. Show all issues

It'd be nice to break this up into a few smaller paragraphs. Each conditional here ("If a position variable is specified ..." should probably be its own paragraph, in order to help with scanning the rules.

rbtools/commands/main.py (Diff revision 5)
The issue has been resolved. Show all issues
```
No blank line here.
```

rbtools/commands/main.py (Diff revision 5)

The issue has been resolved. Show all issues

During my first read, it wasn't immediately clear about how this would affect things. OSError is so general that I had to look up how subprocess used it. It seems it'll only be raised primarily for "File not found" errors, and that commands themselves won't trigger them.

Might be nice just for future readers to add a comment saying that if we got an OSError, it's because the above command most likely does not exist, and we'll fall through to checking aliases.

rbtools/utils/aliases.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Should be on the same line.
```

rbtools/utils/aliases.py (Diff revision 5)

The issue has been resolved. Show all issues

Where is $* handled?

brennie Oct. 27, 2014, 9:30 a.m.

Oops I meant to remove that. I couldn't come up for a good way to have $* handled intelligently without writing a full blown parser.

brennie Oct. 27, 2014, 2:18 p.m.

The latest diff (which will be up shortly) supports $*

rbtools/utils/aliases.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Can this mention that it'll handle argument replacement?
```

rbtools/utils/tests.py (Diff revision 5)

The issue has been resolved. Show all issues

I don't think this tests rbtools.api anymore. Probably an old comment.

How about just saying "rbtools.utils"?

Change Summary:

Add support for $* which expands to all arguments

Commit:

e6c473089cbc8f9138fa2dad960b8aee461bbbb7

09308fd2d00911f49638c697f4af8a3cde341bd3

Diff:

Revision 6 (+257 -17)

Show changes

	docs/rbtools/index.rst
	docs/rbtools/rbt/aliases.rst
	rbtools/commands/main.py
	rbtools/utils/aliases.py
	rbtools/utils/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/main.py
    rbtools/utils/tests.py
    rbtools/utils/aliases.py

Ignored Files:
    docs/rbtools/rbt/aliases.rst
    docs/rbtools/index.rst



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/main.py
    rbtools/utils/tests.py
    rbtools/utils/aliases.py

Ignored Files:
    docs/rbtools/rbt/aliases.rst
    docs/rbtools/index.rst

rbtools/utils/aliases.py (Diff revision 6)
The issue has been resolved. Show all issues
```
 'pipes' imported but unused
```
rbtools/utils/tests.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 56
 E231 missing whitespace after ','
```
rbtools/utils/tests.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 1
 W391 blank line at end of file
```

Commit:

09308fd2d00911f49638c697f4af8a3cde341bd3

bb313f1859565316d3c3ab5c888dbad291ea1465

Diff:

Revision 7 (+255 -17)

Show changes

	docs/rbtools/index.rst
	docs/rbtools/rbt/aliases.rst
	rbtools/commands/main.py
	rbtools/utils/aliases.py
	rbtools/utils/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/main.py
    rbtools/utils/tests.py
    rbtools/utils/aliases.py

Ignored Files:
    docs/rbtools/rbt/aliases.rst
    docs/rbtools/index.rst



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/main.py
    rbtools/utils/tests.py
    rbtools/utils/aliases.py

Ignored Files:
    docs/rbtools/rbt/aliases.rst
    docs/rbtools/index.rst

Commit:

bb313f1859565316d3c3ab5c888dbad291ea1465

cc5c043d0c707d4cb064b8fb2e16a485c48ec130

Diff:

Revision 8 (+256 -17)

Show changes

	docs/rbtools/index.rst
	docs/rbtools/rbt/aliases.rst
	rbtools/commands/main.py
	rbtools/utils/aliases.py
	rbtools/utils/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/main.py
    rbtools/utils/tests.py
    rbtools/utils/aliases.py

Ignored Files:
    docs/rbtools/rbt/aliases.rst
    docs/rbtools/index.rst



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/main.py
    rbtools/utils/tests.py
    rbtools/utils/aliases.py

Ignored Files:
    docs/rbtools/rbt/aliases.rst
    docs/rbtools/index.rst

rbtools/utils/aliases.py (Diff revision 8)

The issue has been dropped. Show all issues

Only one blank line between functions.

brennie Oct. 29, 2014, 5:48 p.m.

One line between functions applies to functions within classes. All top-level functions and classes should have 2 spaces between them as per PEP8.

justy777 Oct. 29, 2014, 6:15 p.m.

My apologies, I'm gonna read PEP8 more closely.

rbtools/utils/aliases.py (Diff revision 8)
The issue has been dropped. Show all issues
```
Only one blank line between functions.
```
rbtools/utils/aliases.py (Diff revision 8)
The issue has been dropped. Show all issues
```
And here.
```

rbtools/utils/aliases.py (Diff revision 8)

The issue has been resolved. Show all issues

This seems like it could be much cleaner as a regex.

rbtools/utils/aliases.py (Diff revision 8)
The issue has been resolved. Show all issues
```
You don't need this because of the final return call.
```

Change Summary:

Add the _split_cmd function to not rely on shlex's behaviour.

Commit:

cc5c043d0c707d4cb064b8fb2e16a485c48ec130

5625ccc7db29b9341f2eb2d7fb16e017468202d7

Diff:

Revision 9 (+320 -17)

Show changes

	docs/rbtools/index.rst
	docs/rbtools/rbt/aliases.rst
	rbtools/commands/main.py
	rbtools/utils/aliases.py
	rbtools/utils/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/main.py
    rbtools/utils/tests.py
    rbtools/utils/aliases.py

Ignored Files:
    docs/rbtools/rbt/aliases.rst
    docs/rbtools/index.rst



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/main.py
    rbtools/utils/tests.py
    rbtools/utils/aliases.py

Ignored Files:
    docs/rbtools/rbt/aliases.rst
    docs/rbtools/index.rst

rbtools/utils/aliases.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
rbtools/utils/aliases.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Col: 24
 E711 comparison to None should be 'if cond is None:'
```
rbtools/utils/aliases.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Change Summary:

Fix reviewbot's issues

Commit:

5625ccc7db29b9341f2eb2d7fb16e017468202d7

9aefe25c63c6c31c4219ba9c109d417244972d07

Diff:

Revision 10 (+321 -17)

Show changes

	docs/rbtools/index.rst
	docs/rbtools/rbt/aliases.rst
	rbtools/commands/main.py
	rbtools/utils/aliases.py
	rbtools/utils/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/main.py
    rbtools/utils/tests.py
    rbtools/utils/aliases.py

Ignored Files:
    docs/rbtools/rbt/aliases.rst
    docs/rbtools/index.rst



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/main.py
    rbtools/utils/tests.py
    rbtools/utils/aliases.py

Ignored Files:
    docs/rbtools/rbt/aliases.rst
    docs/rbtools/index.rst

rbtools/utils/aliases.py (Diff revision 10)

The issue has been resolved. Show all issues

This scares me :) So easy to have really bad bugs in command line string parsing.

Can we leverage shlex.split() for any of this?

chipx86 Nov. 1, 2014, 4:43 p.m.

Or shlex in general.

By the way, we decided we'll be dropping Python 2.5 for the release that this will go into (RBTools 0.7), which means you'll have access to the posix= parameter.

brennie Nov. 1, 2014, 6:26 p.m.

I tried hard to leverage shlex for this but in the end I couldn't.

Using shlex with posix=True will split the arguments correctly, yes, but it will also strip the quotes from quoted substrings, so there will be now way to know if the inner string was quoted. Knowing if a substirng is quoted is important for how I interpreted how the $* paramter should work. If the $* parameter is quoted, it is treated how $@ is in bash -- as a single paramter; however, if it is not quoted, it is treated as serveral different parameters. Should I keep both behaviours, or just use shlex with posix=True and have a single behaviour (yielding different paramters)?

rbtools/utils/tests.py (Diff revision 10)
The issue has been resolved. Show all issues
```
No periods in test docstrings.
Same below.
```

Commit:

9aefe25c63c6c31c4219ba9c109d417244972d07

7f05483a50e3ce7588a94b2048ebdb6c8bd19c69

Diff:

Revision 11 (+221 -17)

Show changes

	docs/rbtools/index.rst
	docs/rbtools/rbt/aliases.rst
	rbtools/commands/main.py
	rbtools/utils/aliases.py
	rbtools/utils/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/main.py
    rbtools/utils/tests.py
    rbtools/utils/aliases.py

Ignored Files:
    docs/rbtools/rbt/aliases.rst
    docs/rbtools/index.rst



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/main.py
    rbtools/utils/tests.py
    rbtools/utils/aliases.py

Ignored Files:
    docs/rbtools/rbt/aliases.rst
    docs/rbtools/index.rst

Change Summary:

Remove posix paramter from shlex.split; it defaults to true

Commit:

7f05483a50e3ce7588a94b2048ebdb6c8bd19c69

7816b0dfb11fba3b65a560628f18c060da8ff638

Diff:

Revision 12 (+221 -17)

Show changes

	docs/rbtools/index.rst
	docs/rbtools/rbt/aliases.rst
	rbtools/commands/main.py
	rbtools/utils/aliases.py
	rbtools/utils/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/main.py
    rbtools/utils/tests.py
    rbtools/utils/aliases.py

Ignored Files:
    docs/rbtools/rbt/aliases.rst
    docs/rbtools/index.rst



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/main.py
    rbtools/utils/tests.py
    rbtools/utils/aliases.py

Ignored Files:
    docs/rbtools/rbt/aliases.rst
    docs/rbtools/index.rst

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to master (66b589c)