Add support to `rbtools` for command aliases.

Review Request #6491 — Created Oct. 22, 2014 and submitted

brennie
RBTools
master
7816b0d...
rbtools, students

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.

Unit tests pass.

  • 0
  • 0
  • 27
  • 4
  • 31
Description From Last Updated
  1. 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
    
    
  2. rbtools/commands/main.py (Diff revision 1)
     
     
     'StringIO' imported but unused
    
  3. rbtools/utils/aliases.py (Diff revision 1)
     
     
     'logging' imported but unused
    
  4. rbtools/utils/aliases.py (Diff revision 1)
     
     
     'sys' imported but unused
    
  5. 
      
brennie
  1. 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
    
    
  2. rbtools/commands/main.py (Diff revision 2)
     
     
     'StringIO' imported but unused
    
  3. 
      
brennie
  1. 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
    
    
  2. 
      
brennie
  1. 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
    
    
  2. 
      
david
  1. 
      
  2. rbtools/commands/main.py (Diff revision 4)
     
     

    There should still be two blank lines here.

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

    Alignment is off here.

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

    How about:

    try:
        return args[index]
    except IndexError:
        return ''
    
  5. rbtools/utils/aliases.py (Diff revision 4)
     
     

    No need for this empty line.

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

    How about:

    use_shell = alias.startswith('!')
    if use_shell:
        cmd = shlex.split(alias[1:])
    else:
        cmd = [RB_MAIN] + shlex.split(alias)
    
  7. 
      
brennie
  1. 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
    
    
  2. 
      
chipx86
  1. 
      
  2. docs/rbtools/rbt/aliases.rst (Diff revision 5)
     
     
     
     

    Two blank lines between headers.

    Same below.

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

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

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

    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.

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

    No blank line here.

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

    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.

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

    Should be on the same line.

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

    Where is $* handled?

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

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

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

    Can this mention that it'll handle argument replacement?

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

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

    How about just saying "rbtools.utils"?

  11. 
      
brennie
  1. 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
    
    
  2. rbtools/utils/aliases.py (Diff revision 6)
     
     
     'pipes' imported but unused
    
  3. rbtools/utils/tests.py (Diff revision 6)
     
     
    Col: 56
     E231 missing whitespace after ','
    
  4. rbtools/utils/tests.py (Diff revision 6)
     
     
    Col: 1
     W391 blank line at end of file
    
  5. 
      
brennie
  1. 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
    
    
  2. 
      
brennie
  1. 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
    
    
  2. 
      
  1. 
      
  2. rbtools/utils/aliases.py (Diff revision 8)
     
     
     
     

    Only one blank line between functions.

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

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

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

    Only one blank line between functions.

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

    And here.

  5. 
      
david
  1. 
      
  2. rbtools/utils/aliases.py (Diff revision 8)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

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

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

    You don't need this because of the final return call.

  4. 
      
brennie
  1. 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
    
    
  2. rbtools/utils/aliases.py (Diff revision 9)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  3. rbtools/utils/aliases.py (Diff revision 9)
     
     
    Col: 24
     E711 comparison to None should be 'if cond is None:'
    
  4. rbtools/utils/aliases.py (Diff revision 9)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  5. 
      
brennie
  1. 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
    
    
  2. 
      
chipx86
  1. 
      
  2. rbtools/utils/aliases.py (Diff revision 10)
     
     

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

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

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

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

  3. rbtools/utils/tests.py (Diff revision 10)
     
     

    No periods in test docstrings.

    Same below.

  4. 
      
brennie
  1. 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
    
    
  2. 
      
brennie
  1. 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
    
    
  2. 
      
chipx86
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (66b589c)
Loading...