Use "rbt" instead of "rb" for base command.

Review Request #3727 — Created Jan. 6, 2013 and submitted

Information

RBTools
api

Reviewers

This work is on top of John Sintal's work on several commands. Once
This has been reviewed, both will be pushed together. The command
files have not been modified, just renamed.

Use "rbt" instead of "rb" for base command.

The base command "rb" conflicted with another package, so to avoid
problems we will now use "rbt" instead. I refactored things a bit to
make changing this name easier in the future.

Add Steven MacLeod and John Sintal to AUTHORS


Remove unused files.

This removes a number of unused command and api files which have been
sitting around. It's possible some of the logic present in the command
files could be useful in the future, but the implementation details
would need to be changed drastically.

 
Description From Last Updated

Can you use 'f' instead of 'opened_file"? It's more common.

chipx86chipx86

Two blank lines.

chipx86chipx86

The """ should be on the next line.

chipx86chipx86

Is there any need to store these in self?

chipx86chipx86

Can just be: if not diff: Or maybe just "if diff" and reverse the two clauses.

chipx86chipx86

It's possible that an ImportError is due to something successfully imported being unable to import something else. A stack trace …

chipx86chipx86

This feels like a hack, and possibly a bad assumption. We should specifically get the latest diff, not assume total_results …

chipx86chipx86

We should call diffs.get_item(patch_id) only once.

chipx86chipx86

Too much of a mix of imports/froms

chipx86chipx86

Col: 27 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Probably no need to say "the web interface," because the command line would suffice too.

chipx86chipx86

Should explicitly say "review request" instead of "request"

chipx86chipx86

Col: 23 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Look at the equivalent code in post-review. I believe that this code was just recently changed to handle some stdout/stderr …

chipx86chipx86

The 'pass' is not needed.

chipx86chipx86

Any reason to store in self?

chipx86chipx86

Col: 27 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 23 E127 continuation line over-indented for visual indent

reviewbotreviewbot
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        rbtools/commands/main.py
        rbtools/commands/__init__.py
        rbtools/commands/close.py
        rbtools/commands/patch.py
        rbtools/commands/post.py
        rbtools/commands/attach.py
        rbtools/commands/publish.py
        setup.py
        rbtools/commands/diff.py
      Ignored Files:
        rbtools/api/settings.py
        rbtools/api/serverinterface.py
        rbtools/commands/rbinfo.py
        rbtools/commands/rbinfov2.py
        rbtools/knownissues.txt
        rbtools/commands/rbattach.py
        rbtools/commands/rbupload.py
        rbtools/commands/rb.py
        rbtools/commands/utils.py
        rbtools/commands/tests.py
        rbtools/commands/rbdiff.py
        rbtools/commands/rbcreate.py
        rbtools/commands/rbconfig.py
        rbtools/commands/rbpatch.py
        AUTHORS
        rbtools/commands/rbpublish.py
        rbtools/commands/resource-browser.py
        rbtools/commands/rbopen.py
        rbtools/commands/rbclose.py
        rbtools/commands/rbpost.py
    
    
  2. rbtools/commands/post.py (Diff revision 1)
     
     
    Show all issues
    Col: 27
     E127 continuation line over-indented for visual indent
    
    1. This is fixed in the review request which comes after.
  3. rbtools/commands/post.py (Diff revision 1)
     
     
    Show all issues
    Col: 23
     E127 continuation line over-indented for visual indent
    
  4. 
      
chipx86
  1. Ship It!
  2. 
      
chipx86
  1. 
      
  2. rbtools/commands/attach.py (Diff revision 1)
     
     
     
     
    Show all issues
    Can you use 'f' instead of 'opened_file"? It's more common.
  3. rbtools/commands/close.py (Diff revision 1)
     
     
     
     
    Show all issues
    Two blank lines.
  4. rbtools/commands/close.py (Diff revision 1)
     
     
    Show all issues
    The """ should be on the next line.
  5. rbtools/commands/close.py (Diff revision 1)
     
     
    Show all issues
    Is there any need to store these in self?
  6. rbtools/commands/diff.py (Diff revision 1)
     
     
    Show all issues
    Can just be:
    
    if not diff:
    
    
    Or maybe just "if diff" and reverse the two clauses.
  7. rbtools/commands/main.py (Diff revision 1)
     
     
     
    Show all issues
    It's possible that an ImportError is due to something successfully imported being unable to import something else. A stack trace might be helpful here.
    
    We may want to use Python logging for such things.
    1. Errors and output is being handled in different ways all over the place (calls to sys.stderr.write, die, print, sys.exit etc.). I'm going to do a sweep to standardize things with logging, and post a new review request for it. I've added a comment here to remind me about the strack trace though.
  8. rbtools/commands/patch.py (Diff revision 1)
     
     
     
    Show all issues
    This feels like a hack, and possibly a bad assumption. We should specifically get the latest diff, not assume total_results is numerically the same as the latest ID.
    1. I *think* we're safe here, but I can see why you thought we
      weren't, I thought it was wrong as well for a couple minutes.
      
      If you rename 'patch_id' to 'patch_revision' (which is what
      that variable is actually representing), it should look
      correct (diffs use the revision as their identifier in the
      api urls, and thus the following 'get_item' call will work).
      
      So, with this in mind, the actual assumption being made here
      is that the api items are sorted in ascending order by diff
      revision, and no number is missing from the list, which
      appears to be true. In order to not use this assumption, we
      would have to iterate over every diff, and keep track of the
      max revision. So I think we're fine to use this assumption.
      
      That being said, the variable naming used here is definitely
      confusing, and assumptions like this should be commented. I'll
      rework this section a bit and make it more clear.
      
      Thanks for bringing this up.
  9. rbtools/commands/patch.py (Diff revision 1)
     
     
     
    Show all issues
    We should call diffs.get_item(patch_id) only once.
  10. rbtools/commands/post.py (Diff revision 1)
     
     
     
     
     
     
     
     
    Show all issues
    Too much of a mix of imports/froms
    1. This is fixed in /r/3726
  11. rbtools/commands/post.py (Diff revision 1)
     
     
     
    Show all issues
    Probably no need to say "the web interface," because the command line would suffice too.
  12. rbtools/commands/post.py (Diff revision 1)
     
     
     
    Show all issues
    Should explicitly say "review request" instead of "request"
  13. rbtools/commands/post.py (Diff revision 1)
     
     
     
    Show all issues
    Look at the equivalent code in post-review. I believe that this code was just recently changed to handle some stdout/stderr issues.
    1. I'm going to deal with all the post-review bug fixes when I merge master into api. This will be coming in another review request.
  14. rbtools/commands/post.py (Diff revision 1)
     
     
     
    Show all issues
    The 'pass' is not needed.
  15. rbtools/commands/publish.py (Diff revision 1)
     
     
     
     
    Show all issues
    Any reason to store in self?
  16. 
      
SM
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        rbtools/commands/main.py
        rbtools/commands/__init__.py
        rbtools/commands/diff.py
        rbtools/commands/patch.py
        rbtools/commands/post.py
        rbtools/commands/attach.py
        rbtools/commands/publish.py
        setup.py
        rbtools/commands/close.py
      Ignored Files:
        rbtools/api/serverinterface.py
        rbtools/api/settings.py
        rbtools/commands/rbinfo.py
        rbtools/commands/rbinfov2.py
        rbtools/knownissues.txt
        rbtools/commands/rbattach.py
        rbtools/commands/rbupload.py
        rbtools/commands/rb.py
        rbtools/commands/utils.py
        rbtools/commands/tests.py
        rbtools/commands/rbdiff.py
        rbtools/commands/rbcreate.py
        rbtools/commands/rbconfig.py
        rbtools/commands/rbpatch.py
        AUTHORS
        rbtools/commands/rbpublish.py
        rbtools/commands/resource-browser.py
        rbtools/commands/rbopen.py
        rbtools/commands/rbclose.py
        rbtools/commands/rbpost.py
    
    
  2. rbtools/commands/post.py (Diff revision 2)
     
     
    Show all issues
    Col: 27
     E127 continuation line over-indented for visual indent
    
  3. rbtools/commands/post.py (Diff revision 2)
     
     
    Show all issues
    Col: 23
     E127 continuation line over-indented for visual indent
    
  4. 
      
chipx86
  1. Ship It!
  2. 
      
SM
Review request changed
Status:
Completed
Change Summary:
Merged into api (2b12805a6b392e85f4a6e21424e0628736c1b405).