Add a get_change method to SCMTool and HostingService

Review Request #4284 — Created July 3, 2013 and submitted

Information

Review Board
master

Reviewers

Add a get_change method to SCMTool and HostingService

This change adds the final piece of integration API on the backend that we need
for post-commit reviews--the ability to fetch the commit message and diff from
the version control system given a revision ID. This is implemented for SVN and
GitHub, just like the get_branches and get_commits methods.
- Ran unit tests.
- Used this with my post-commit UI for both github and svn repos.
Description From Last Updated

Rather than building a big string, we should build an array of lines. It's more efficient.

chipx86chipx86

No blank line.

chipx86chipx86

No blank line.

chipx86chipx86

Hopefully we never get files with \r\n, or everything above will fail. I don't know if we can, but it …

chipx86chipx86

Thinking we might want to be more future-proof and structure this so we could add more later than a message …

chipx86chipx86

I don't think we need this anymore. That was only important for Python 2.4.

chipx86chipx86

This is hard to read. Can we break them up by lines? I know we don't have to deal with …

chipx86chipx86

I was really unsure what was going on at first with 'testcase' and 'self'. Rather than have a testcase variable, …

chipx86chipx86

No blank line. Same below.

chipx86chipx86

""" on the next line.

chipx86chipx86

""" on the next line.

chipx86chipx86

Can you use prefix="reviewboard-svn."? This will make it easier to locate stale temp directories.

chipx86chipx86

local variable 'testcase' is assigned to but never used

reviewbotreviewbot

Col: 32 E201 whitespace after '{'

reviewbotreviewbot

Col: 59 E202 whitespace before '}'

reviewbotreviewbot

Col: 35 E201 whitespace after '{'

reviewbotreviewbot

Col: 66 E202 whitespace before '}'

reviewbotreviewbot

Col: 29 E201 whitespace after '['

reviewbotreviewbot

Col: 31 E201 whitespace after '{'

reviewbotreviewbot

Col: 49 E202 whitespace before '}'

reviewbotreviewbot

Col: 51 E202 whitespace before ']'

reviewbotreviewbot

Col: 30 E201 whitespace after '{'

reviewbotreviewbot

Col: 46 E202 whitespace before '}'

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 9 E301 expected 1 blank line, found 0

reviewbotreviewbot

I think this is actually worse. I was hoping something indented in the proper indentation level that just wraps sanely. …

chipx86chipx86

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 80 E501 line too long (99 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (99 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (102 > 79 characters)

reviewbotreviewbot
chipx86
  1. 
      
  2. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
    Show all issues
    Rather than building a big string, we should build an array of lines. It's more efficient.
  3. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
     
    Show all issues
    No blank line.
  4. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
     
    Show all issues
    No blank line.
  5. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
    Show all issues
    Hopefully we never get files with \r\n, or everything above will fail. I don't know if we can, but it seems possible.
    1. Hrm. So github seems to be giving us \n on everything I've tested with, but admittedly, most people aren't committing windows-formatted files. I'm not sure if it's possible to get this or not, and their documentation doesn't say. Maybe just wait for the bug report? :P
    2. I guess :/ Hopefully GitHub handles this for us.
      
      Since this is not a new problem though, and RBTools has to build diffs too, I wonder if we should consider making some little independent Python module for sanely building diffs based on input, and handling all the annoying normalization stuff. Could be useful. Even if it's very small.
    3. It looks like rbtools does something similar to what this does--the diff headers lines are wrapped with a single \n, and doesn't make any changes to the patch. The perforce client in rbtools does have a special case for broken files that have \r\r\n, but I think that's only a problem when the file in the depot has \r\n but is a text format, and it does the diff on windows and tries to add additional \r's, which shouldn't be an issue with github.
    4. Sounds fine then.
  6. reviewboard/hostingsvcs/github.py (Diff revision 1)
     
     
    Show all issues
    Thinking we might want to be more future-proof and structure this so we could add more later than a message and diff without just tacking onto tuples.
    
    We have a ChangeSet object in scmtools.core. I wonder if we can evolve that.
    1. I'm going to build on the "Commit" object.
  7. reviewboard/hostingsvcs/tests.py (Diff revision 1)
     
     
     
    Show all issues
    I don't think we need this anymore. That was only important for Python 2.4.
    1. I'll go through in a separate change and clean up all of these.
  8. reviewboard/hostingsvcs/tests.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    This is hard to read. Can we break them up by lines? I know we don't have to deal with the contents much, but it'd make debugging and reading comprehension so much nicer.
  9. reviewboard/hostingsvcs/tests.py (Diff revision 1)
     
     
     
     
     
    Show all issues
    I was really unsure what was going on at first with 'testcase' and 'self'. Rather than have a testcase variable, can we instead call the inner 'self' something like 'service'? This is the pattern some other tests have.
  10. reviewboard/hostingsvcs/tests.py (Diff revision 1)
     
     
     
    Show all issues
    No blank line.
    
    Same below.
  11. reviewboard/scmtools/models.py (Diff revision 1)
     
     
    Show all issues
    """ on the next line.
  12. reviewboard/scmtools/svn.py (Diff revision 1)
     
     
    Show all issues
    """ on the next line.
  13. reviewboard/scmtools/svn.py (Diff revision 1)
     
     
    Show all issues
    Can you use prefix="reviewboard-svn."? This will make it easier to locate stale temp directories.
  14. 
      
david
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/scmtools/models.py
        reviewboard/scmtools/tests.py
        reviewboard/scmtools/core.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/github.py
        reviewboard/hostingsvcs/tests.py
        reviewboard/scmtools/svn.py
      Ignored Files:
    
    
  2. reviewboard/hostingsvcs/tests.py (Diff revision 2)
     
     
    Show all issues
    Col: 32
     E201 whitespace after '{'
    
  3. reviewboard/hostingsvcs/tests.py (Diff revision 2)
     
     
    Show all issues
    Col: 59
     E202 whitespace before '}'
    
  4. reviewboard/hostingsvcs/tests.py (Diff revision 2)
     
     
    Show all issues
    Col: 35
     E201 whitespace after '{'
    
  5. reviewboard/hostingsvcs/tests.py (Diff revision 2)
     
     
    Show all issues
    Col: 66
     E202 whitespace before '}'
    
  6. reviewboard/hostingsvcs/tests.py (Diff revision 2)
     
     
    Show all issues
    Col: 29
     E201 whitespace after '['
    
  7. reviewboard/hostingsvcs/tests.py (Diff revision 2)
     
     
    Show all issues
    Col: 31
     E201 whitespace after '{'
    
  8. reviewboard/hostingsvcs/tests.py (Diff revision 2)
     
     
    Show all issues
    Col: 49
     E202 whitespace before '}'
    
  9. reviewboard/hostingsvcs/tests.py (Diff revision 2)
     
     
    Show all issues
    Col: 51
     E202 whitespace before ']'
    
  10. reviewboard/hostingsvcs/tests.py (Diff revision 2)
     
     
    Show all issues
    Col: 30
     E201 whitespace after '{'
    
  11. reviewboard/hostingsvcs/tests.py (Diff revision 2)
     
     
    Show all issues
    Col: 46
     E202 whitespace before '}'
    
  12. reviewboard/hostingsvcs/tests.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  13. reviewboard/hostingsvcs/tests.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  14. reviewboard/hostingsvcs/tests.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  15. reviewboard/hostingsvcs/tests.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  16. reviewboard/hostingsvcs/tests.py (Diff revision 2)
     
     
    Show all issues
    Col: 9
     E301 expected 1 blank line, found 0
    
  17. 
      
reviewbot
  1. This is a review from Review Bot.
      Tool: Pyflakes
      Processed Files:
        reviewboard/scmtools/models.py
        reviewboard/scmtools/tests.py
        reviewboard/scmtools/core.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/github.py
        reviewboard/hostingsvcs/tests.py
        reviewboard/scmtools/svn.py
      Ignored Files:
    
    
  2. reviewboard/hostingsvcs/tests.py (Diff revision 2)
     
     
    Show all issues
     local variable 'testcase' is assigned to but never used
    
  3. 
      
david
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/scmtools/models.py
        reviewboard/scmtools/tests.py
        reviewboard/scmtools/core.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/github.py
        reviewboard/hostingsvcs/tests.py
        reviewboard/scmtools/svn.py
      Ignored Files:
    
    
  2. reviewboard/hostingsvcs/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  3. reviewboard/hostingsvcs/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  4. reviewboard/hostingsvcs/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  5. reviewboard/hostingsvcs/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  6. 
      
reviewbot
  1. This is a review from Review Bot.
      Tool: Pyflakes
      Processed Files:
        reviewboard/scmtools/models.py
        reviewboard/scmtools/tests.py
        reviewboard/scmtools/core.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/github.py
        reviewboard/hostingsvcs/tests.py
        reviewboard/scmtools/svn.py
      Ignored Files:
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/hostingsvcs/tests.py (Diff revision 3)
     
     
     
     
     
    Show all issues
    I think this is actually worse. I was hoping something indented in the proper indentation level that just wraps sanely. Between what we had and this, though, I'd take what we had.
    1. Okay, I think I have a nice solution. It changes the diff slightly because it doesn't allow the lines with 1 trailing space, but it still tests that we pull the data out of the API response and assemble it into a full diff.
  3. 
      
david
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/scmtools/models.py
        reviewboard/scmtools/tests.py
        reviewboard/scmtools/core.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/github.py
        reviewboard/hostingsvcs/tests.py
        reviewboard/scmtools/svn.py
      Ignored Files:
    
    
  2. reviewboard/hostingsvcs/tests.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (99 > 79 characters)
    
  3. reviewboard/hostingsvcs/tests.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (99 > 79 characters)
    
  4. reviewboard/hostingsvcs/tests.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (102 > 79 characters)
    
  5. 
      
reviewbot
  1. This is a review from Review Bot.
      Tool: Pyflakes
      Processed Files:
        reviewboard/scmtools/models.py
        reviewboard/scmtools/tests.py
        reviewboard/scmtools/core.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/github.py
        reviewboard/hostingsvcs/tests.py
        reviewboard/scmtools/svn.py
      Ignored Files:
    
    
  2. 
      
chipx86
  1. Ship It!
  2. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to master (0516813).