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)
     
     
    With the above, this would be "%(name)s"
  3. rbtools/commands/rb.py (Diff revision 1)
     
     
     
     
    No blank line here. optparse is part of Python.
  4. rbtools/commands/rb.py (Diff revision 1)
     
     
    "Available"
  5. rbtools/commands/rb.py (Diff revision 1)
     
     
    No need for parens around the format arguments when there's just one value.
  6. rbtools/commands/rb.py (Diff revision 1)
     
     
    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)
     
     
    No need for parens. Should use { "name": args[0] }
  8. rbtools/commands/rb.py (Diff revision 1)
     
     
    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)
     
     
     
    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)
     
     
    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)
     
     
    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)
     
     
     
    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)
     
     
     
    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)
     
     
    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)
     
     
    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: Closed (submitted)

Change Summary:

Committed with some text changes to api (9158683)
Loading...