make post-review able to handle customized reviewboard URL namespaces

Review Request #360 — Created April 17, 2008 and submitted

edgimar
Review Board SVN (deprecated)
457
reviewboard
This patch allows the post-review tool to work in cases where the reviewboard URL contains a prefix (e.g. http://my.server.com/reviewboard/...). 

I also cleaned up / added some documentation to the script.
Diffs were successfully posted to a 'prefixed' server path with it, as well as to this server.
chipx86
  1. Thanks for doing this. Only a couple small comments.
    1. Can you add "457" to the bugs field on this review request so we remember to close it when this gets committed?
      
      Thanks.
  2. I'm not sure we want to enforce a trailing slash. Repository paths are not necessarily always going to be directories.
    1. This may be true.  The issue is that the urljoin function will strip off of its left argument anything to the right of the last '/'.  Perhaps urljoin is the wrong function to use?  Or the user should be warned in a comment near the REVIEWBOARD_URL assignment that the trailing slash is necessary.
    2. I think we should probably move this code to the repository checking. We can normalize the URL and make sure the trailing slashes match. A quick way of doing this is to look up both variants when trying to find the database entry.
    3. Do you mean to the scan_for_server method?  If so, I guess it would mean having to add it to every SCMClient subclass (or a method to SCMClient which was called from each subclass)?  I'll leave it to someone else to implement it.  Feel free to update the diff if you come up with something you like.
    4. Sorry, I wrote this just before leaving for work and didn't explain it well.
      
      I was thinking more on the Review Board side of things. It could be more intelligent with its server lookups so that we only have to do this in one place.
  3. We could still combine these into one path.
    
    review_url = urljoin(server.url, "r/%s" % review_request['id'])
    1. I have no objections to doing that.
  4. 
      
Loading...