Review Board

beta

Rework post-review for SVN support and several new options

Updated 1 year, 6 months ago

Christian Hammond Reviewers
trunk reviewboard
None Review Board SVN
Restructured much of post-review in order to clean up the code and to add both SVN support (from /r/89/) and Perforce support to the same script. This will allow support for CVS and other backends down the road.

We now check the user's $HOME/.reviewboardrc for the Review Board server used for the particular tree the user is in. If not found, we check up the directory hierarchy for a .reviewboardrc and use the one specified in that, if found. If not, we perform logic specific to the SCMTool. For example, SVN will check reviewboard:url properties in the directories up to the top of the local checkout and then check the repository root for the server.

This also adds support for specifying default reviewers on the command line and for updating an existing review request for systems that don't have changeset support (like svn) by using the -r (or --review-request-id) parameter.
Tried the new options and tried against a Perforce and a SVN repository. Tried updating an existing review request with -r. Posted this review request using the new post-review.
Ship it!
Posted 1 year, 6 months ago (July 8th, 2007, 1:26 p.m.)
Just a cursory glance; I'm assuming all the actual logic works, since you've been testing it.
Loading diff fragment...
Split onto separate lines
  1. Christian Hammond 1 year, 6 months ago (July 8th, 2007, 3:43 p.m.)
    Done.
Loading diff fragment...
You can write this as "if 'USERPROFILE' in os.environ:"
Which makes it sexy.  Yeah.
  1. Christian Hammond 1 year, 6 months ago (July 8th, 2007, 3:43 p.m.)
    Good point. Fixed all instances.
Loading diff fragment...
I'm not sure I like calling this "SCMTool".  It isn't the same thing as our normal SCMTool instances, so it's a bit confusing.

Maybe "SCMClient", "SVNClient", etc.?
  1. Christian Hammond 1 year, 6 months ago (July 8th, 2007, 3:44 p.m.)
    Much better name. Done.
Loading diff fragment...
Can you add a comment suggesting why these are commented out?
  1. Christian Hammond 1 year, 6 months ago (July 8th, 2007, 3:44 p.m.)
    Yep.