Base command class and 'rb post' command.

Review Request #3449 — Created Oct. 24, 2012 and submitted

Information

RBTools
api

Reviewers

This includes the beginnings of the new base class for rb commands,
and the rb post command.

'rb post' is meant to eventually replace post-review for the common
task of generating a diff and creating/updating a review request. It
has many of the extra options of post-review stripped out, to make
it more focused.

"rb <command>" now uses the new entry points, expecting subclasses
of ``Command``. If the command isn't found it will attempt to
execute "rb-<command>" on the system.
Local testing with git repositories. This review request was posted using 'rb post'
Description From Last Updated

Blank line between these. (Before/after if statements or other blocks when bunched up against code on the same indent level.)

chipx86chipx86

Blank line between these.

chipx86chipx86

Blank line between these.

chipx86chipx86

Is that 'break' on the right indentation level?

chipx86chipx86

Indentation looks off on the 2nd and 3rd line.

chipx86chipx86

In this case, just use \ instead of parens.

chipx86chipx86

Blank line.

chipx86chipx86

Blank line.

chipx86chipx86

Can just use \ in this case.

chipx86chipx86

Probably should just do 'r/%s/' % .... That saves you the str() and is more readable.

chipx86chipx86

Can you build the filename first? It'll make this less awkward.

chipx86chipx86

Blank line should remain.

chipx86chipx86
chipx86
  1. Some of my comments here may really be applying to older code that was just ported over. I'm not sure what's new and what isn't. But while you're here... :)
    
    Overall this looks good. I mostly have some stylistic stuff (personal pet peeves of mine, mostly), with a few actually useful things to say.
  2. rbtools/commands/rb.py (Diff revision 1)
     
     
     
    Blank line between these. (Before/after if statements or other blocks when bunched up against code on the same indent level.)
  3. rbtools/commands/rb.py (Diff revision 1)
     
     
    Do we want to handle the case where this throws some exception?
    1. Yeah, that's probably a good idea. I've added handling for when the entry point can't be loaded, and a catch all for any exceptions the command throws when initializing.
  4. rbtools/commands/rb.py (Diff revision 1)
     
     
     
    Blank line between these.
  5. rbtools/commands/rb.py (Diff revision 1)
     
     
    We probably will want to change things such that stdout and stderr are not captured in this case, and instead are fed directly to the main stdout/stderr.
    
    Possibly need to support stdin too.
    1. Definitely. I've added a TODO for now.
  6. rbtools/commands/rbpost.py (Diff revision 1)
     
     
    Wondering if we should put "The Review Board Project" as the author for all first-party commands, to better distinguish from third-party commands.
    1. Yeah, it will be a little strange if all the commands have various authors tagged.
  7. rbtools/commands/rbpost.py (Diff revision 1)
     
     
     
    Blank line between these.
  8. rbtools/commands/rbpost.py (Diff revision 1)
     
     
     
     
     
     
    Is that 'break' on the right indentation level?
    1. Ah thanks for pointing this out. There's actually a bigger problem here, instead
      of breaking I probably want to raise a StopIteration, to break out of both loops.
      It, of course, should be moved up into the if (where the break should have been).
      
      (Also, self.info needed to be changed to self.repository_info :/)
      
      The code in post-review this is derived from actually has a bug itself; it's only
      grabbing a single page of repositories. So, if there are more than 25 repos, it
      won't check all of them. I fixed this for rb post, but do you think it's worth
      patching in post-review?
  9. rbtools/commands/rbpost.py (Diff revision 1)
     
     
     
     
    Indentation looks off on the 2nd and 3rd line.
    1. What would you like to see here for indentation? I was going for something like this:
      
      review_request = self.api_root.get_review_request(
          values={
              'review_request_id': self.options.rid,
          })
      
      but with the 'values={' moved up. Should it be:
      
      review_request = self.api_root.get_review_request(values={
          'review_request_id': self.options.rid,
      })
    2. Either of those are fine (though in this case, I'd pick the latter). It's just weird when values= is on the first line but then }) isn't aligned with the start of that line.
  10. rbtools/commands/rbpost.py (Diff revision 1)
     
     
     
    In this case, just use \ instead of parens.
  11. rbtools/commands/rbpost.py (Diff revision 1)
     
     
     
    Blank line.
  12. rbtools/commands/rbpost.py (Diff revision 1)
     
     
     
    Blank line.
  13. rbtools/commands/rbpost.py (Diff revision 1)
     
     
     
    Can just use \ in this case.
  14. rbtools/commands/rbpost.py (Diff revision 1)
     
     
    Probably should just do 'r/%s/' % .... That saves you the str() and is more readable.
  15. rbtools/commands/rbpost.py (Diff revision 1)
     
     
     
    Can you build the filename first? It'll make this less awkward.
  16. 
      
SM
chipx86
  1. 
      
  2. setup.py (Diff revision 2)
     
     
     
     
     
    Blank line should remain.
  3. 
      
SM
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to api (4861799146).
Loading...