Capabilities with better moved files support

Review Request #2918 - Created Feb. 26, 2012 and submitted

Dave Druska
RBTools
master
reviewboard
Added post-review client capability checking and git moved-files support.
Tested the following cases:
1. Posted a review with up-to-date RB and rbtools. Moved files were tracked.
2. Posted a review where the RB server has moved_files=False. Moved files were not tracked.
3. Posted a review where the RB server has no "compatabilities". Moved files were not tracked.
4. Posted a review where the RB server has no "moved_files" capability (and again with no "diffs" category). Moved files were not tracked.

Unit tests pass, don't really know how to write tests for this since it requires a server (mock objects?).
  • 4
  • 0
  • 6
  • 0
  • 10
Description From Last Updated
Summary should be on the same line. Since this all fits on one line, just put the text right up ... Christian Hammond Christian Hammond
Should specifically catch a KeyError. Christian Hammond Christian Hammond
Too much repeat code. I'd suggest something like: cmdline = [self.git, "diff", .....] if has_capability(...): cmdline.append('-M') return execute(cmdline) Christian Hammond Christian Hammond
I'd prefer we set the server instead. There may be other useful objects later. Given that, we'd probably want to ... Christian Hammond Christian Hammond
Christian Hammond
  1. This is a good first step, but it'll end up breaking older installs.
    
    What we'll need is to conditionally do this based on server capabilities.
    
    I'd suggest a new singleton API resource, /api/capabilities/, containing a payload that looks like:
    
    {
        'capabilities': {
            'diffs': {
                'moved-files': true
            }
        }
    }
    
    Then post-review can check for the capability. I'd suggest a Capabilities class in rbtools/api/capabilities.py, with a has_capability(category, name) function. If /api/capabilities/ is 404, then a cap defaults to false. Otherwise, try to find it in the the payload (which would be fetched only once).
    
    It's a little more work, but really helps us long-term and makes sure your moved files work won't break on older versions of Review Board.
  2. 
      
Dave Druska
Christian Hammond
  1. 
      
  2. rbtools/api/capabilities.py (Diff revision 2)
     
     
     
     
    Summary should be on the same line. Since this all fits on one line, just put the text right up against the """.
    
    Probably makes more sense to say "Retrieves and stores"
  3. rbtools/api/capabilities.py (Diff revision 2)
     
     
    Should specifically catch a KeyError.
  4. rbtools/clients/git.py (Diff revision 2)
     
     
     
     
     
     
     
    Too much repeat code. I'd suggest something like:
    
    cmdline = [self.git, "diff", .....]
    
    if has_capability(...):
        cmdline.append('-M')
    
    return execute(cmdline)
  5. rbtools/postreview.py (Diff revision 2)
     
     
     
    I'd prefer we set the server instead. There may be other useful objects later.
    
    Given that, we'd probably want to rename get_capabilities() to load_capabilities(), and put that call after check_api_version(). Then things can just access server.capabilities.
  6. 
      
Dave Druska
Dave Druska
  1. 
      
  2. 
      
Christian Hammond
  1. 
      
  2. rbtools/postreview.py (Diff revision 3)
     
     
    Should just call this capabilities, or caps, or something.
  3. rbtools/postreview.py (Diff revision 3)
     
     
     
    I'd imagine this could fit on one line.
    
    if not, perhaps rename capability_list to capabilities or caps.
  4. 
      
Mike Conley
  1. Beyond what Christian has pointed out, this looks good to me.
  2. 
      
Dave Druska
Yazan Medanat
  1. 
      
  2. rbtools/clients/git.py (Diff revision 4)
     
     
     
     
    This could just be return execute(cmdline).
    
    i.e:
    if (self.server and
        self.server.capabilities.has_capability('diffs', 'moved_files')):	
        cmdline.append('-M')
    
    return execute(cmdline)
  3. 
      
Dave Druska
Christian Hammond
  1. 
      
  2. rbtools/api/capabilities.py (Diff revision 5)
     
     
    We always pass a value to this, right? Probably no need for a default.
  3. rbtools/clients/git.py (Diff revision 5)
     
     
    Blank line above this.
  4. rbtools/postreview.py (Diff revision 5)
     
     
    See below on this, but I don't think capabilities should be a default parameter. I think it should always take a value. In that case, just pass caps as a standard argument, not keyword argument. Makes this line read better too :)
  5. 
      
Dave Druska
Dave Druska
Matthew Woehlke
  1. Note: this is related to (the more recently filed) http://code.google.com/p/reviewboard/issues/detail?id=2824; someone with permissions (i.e. Dave) may want to add that to the review details.
  2. 
      
Christian Hammond
  1. Ship It!
  2. 
      
Dave Druska
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (bacdab01f33c75911d03a49ff0115d9d99287a82)
Loading...