• 
      

    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)