Rework 'rb' command, add rb-* commands description

Review Request #2549 — Created Aug. 15, 2011 and submitted

Information

RBTools

Reviewers

Options in 'rb' are parsed by 'optparse' module, added well-formatted help with sub-commands annotations.

 
Description From Last Updated

With the above, this would be "%(name)s"

chipx86chipx86

No blank line here. optparse is part of Python.

chipx86chipx86

"Available"

chipx86chipx86

No need for parens around the format arguments when there's just one value.

chipx86chipx86

This isn't needed if you make RB_COMMANDS a dictionary. You can just do: if RB_MAIN + args[0] in RB_COMMANDS:

chipx86chipx86

No need for parens. Should use { "name": args[0] }

chipx86chipx86

Should avoid map where possible. These are hard to read. Do: _indent = max([len(key) for key in RB_COMMANDS]) - len(RB_MAIN)

chipx86chipx86

This would be nicer as: _COMMANDS_LIST_STR = '\n'.join([ " %s-*s %s" % (_indent, cmd(len(RB_MAIN):], desc) for cmd, desc in RB_COMMANDS.iteritems() …

chipx86chipx86

I don't understand exactly what this is for. Can you add some docstrings on what it's for exactly? I feel …

chipx86chipx86

I'm not sure we want to split, do we? If for some reason there's "rb" elsewhere, this will break. I …

chipx86chipx86

Shouldn't split the fmt into its own string. Instead, just wrap if we need to. Also, instead of %s and …

chipx86chipx86

Description should be on the same line. It can then wrap. (Make sure the description aligns on each line).

chipx86chipx86

Still needs to be documented to explain why it's needed and how it differs from IndentedHelpFormatter.

chipx86chipx86

Do we mean to override _COMMANDS_LIST_STR, or append? Why is this down here instead of up above? It seems an …

chipx86chipx86
chipx86
  1. 
      
  2. rbtools/commands/__init__.py (Diff revision 1)
     
     
    Show all issues
    With the above, this would be "%(name)s"
  3. rbtools/commands/rb.py (Diff revision 1)
     
     
     
     
    Show all issues
    No blank line here. optparse is part of Python.
  4. rbtools/commands/rb.py (Diff revision 1)
     
     
    Show all issues
    "Available"
  5. rbtools/commands/rb.py (Diff revision 1)
     
     
    Show all issues
    No need for parens around the format arguments when there's just one value.
  6. rbtools/commands/rb.py (Diff revision 1)
     
     
    Show all issues
    This isn't needed if you make RB_COMMANDS a dictionary. You can just do:
    
        if RB_MAIN + args[0] in RB_COMMANDS:
  7. rbtools/commands/rb.py (Diff revision 1)
     
     
    Show all issues
    No need for parens. Should use { "name": args[0] }
  8. rbtools/commands/rb.py (Diff revision 1)
     
     
    Show all issues
    Should avoid map where possible. These are hard to read.
    
    Do:
    
    _indent = max([len(key) for key in RB_COMMANDS]) - len(RB_MAIN)
  9. rbtools/commands/rb.py (Diff revision 1)
     
     
     
    Show all issues
    This would be nicer as:
    
    _COMMANDS_LIST_STR = '\n'.join([
        "  %s-*s  %s" % (_indent, cmd(len(RB_MAIN):], desc)
        for cmd, desc in RB_COMMANDS.iteritems()
    ])
  10. rbtools/utils/cmd.py (Diff revision 1)
     
     
    Show all issues
    I don't understand exactly what this is for. Can you add some docstrings on what it's for exactly?
    
    I feel like right now, this should probably just go in rb.py, until we have a good reason to give it its own file. There's no harm in other commands importing from rb.py if needed.
  11. setup.py (Diff revision 1)
     
     
    Show all issues
    I'm not sure we want to split, do we? If for some reason there's "rb" elsewhere, this will break. I think instead we want to do:
    
    if cmd.startswith(RB_MAIN):
        name = cmd[len(RB_MAIN):]
  12. setup.py (Diff revision 1)
     
     
     
    Show all issues
    Shouldn't split the fmt into its own string. Instead, just wrap if we need to.
    
    Also, instead of %s and passing name twice, use "%(name)s" and then have the format arguments be: % { 'name': name }
  13. 
      
MB
chipx86
  1. 
      
  2. rbtools/commands/__init__.py (Diff revision 2)
     
     
     
    Show all issues
    Description should be on the same line. It can then wrap. (Make sure the description aligns on each line).
  3. rbtools/commands/rb.py (Diff revision 2)
     
     
    Show all issues
    Still needs to be documented to explain why it's needed and how it differs from IndentedHelpFormatter.
  4. rbtools/commands/rb.py (Diff revision 2)
     
     
    Show all issues
    Do we mean to override _COMMANDS_LIST_STR, or append?
    
    Why is this down here instead of up above? It seems an odd place for it.
  5. 
      
MB
chipx86
  1. Ship It!
  2. 
      
MB
Review request changed
Status:
Completed
Change Summary:
Committed with some text changes to api (9158683)