Summary

Add a get_change method to SCMTool and HostingService

Review Request #4284 — Created July 3, 2013 and submitted July 5, 2013, 11:15 p.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

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.

Testing Done

- Ran unit tests.
- Used this with my post-commit UI for both github and svn repos.

Issues

Description	From	Last Updated
Rather than building a big string, we should build an array of lines. It's more efficient.	chipx86	July 4, 2013, 5:18 p.m.
No blank line.	chipx86	July 4, 2013, 5:18 p.m.
No blank line.	chipx86	July 4, 2013, 5:18 p.m.
Hopefully we never get files with \r\n, or everything above will fail. I don't know if we can, but it …	chipx86	July 4, 2013, 5:20 p.m.
Thinking we might want to be more future-proof and structure this so we could add more later than a message …	chipx86	July 5, 2013, 2:54 p.m.
I don't think we need this anymore. That was only important for Python 2.4.	chipx86	July 4, 2013, 5:23 p.m.
This is hard to read. Can we break them up by lines? I know we don't have to deal with …	chipx86	July 4, 2013, 5:37 p.m.
I was really unsure what was going on at first with 'testcase' and 'self'. Rather than have a testcase variable, …	chipx86	July 4, 2013, 5:37 p.m.
No blank line. Same below.	chipx86	July 4, 2013, 5:37 p.m.
""" on the next line.	chipx86	July 4, 2013, 5:38 p.m.
""" on the next line.	chipx86	July 4, 2013, 5:38 p.m.
Can you use prefix="reviewboard-svn."? This will make it easier to locate stale temp directories.	chipx86	July 4, 2013, 5:38 p.m.
local variable 'testcase' is assigned to but never used	reviewbot	July 5, 2013, 3:04 p.m.
Col: 32 E201 whitespace after '{'	reviewbot	July 5, 2013, 3:03 p.m.
Col: 59 E202 whitespace before '}'	reviewbot	July 5, 2013, 3:03 p.m.
Col: 35 E201 whitespace after '{'	reviewbot	July 5, 2013, 3:03 p.m.
Col: 66 E202 whitespace before '}'	reviewbot	July 5, 2013, 3:03 p.m.
Col: 29 E201 whitespace after '['	reviewbot	July 5, 2013, 3:03 p.m.
Col: 31 E201 whitespace after '{'	reviewbot	July 5, 2013, 3:03 p.m.
Col: 49 E202 whitespace before '}'	reviewbot	July 5, 2013, 3:03 p.m.
Col: 51 E202 whitespace before ']'	reviewbot	July 5, 2013, 3:03 p.m.
Col: 30 E201 whitespace after '{'	reviewbot	July 5, 2013, 3:03 p.m.
Col: 46 E202 whitespace before '}'	reviewbot	July 5, 2013, 3:03 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	July 5, 2013, 3:03 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	July 5, 2013, 3:03 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	July 5, 2013, 3:03 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	July 5, 2013, 3:03 p.m.
Col: 9 E301 expected 1 blank line, found 0	reviewbot	July 5, 2013, 3:03 p.m.
I think this is actually worse. I was hoping something indented in the proper indentation level that just wraps sanely. …	chipx86	July 5, 2013, 5:48 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	July 5, 2013, 3:17 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	July 5, 2013, 3:17 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	July 5, 2013, 3:17 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	July 5, 2013, 3:17 p.m.
Col: 80 E501 line too long (99 > 79 characters)	reviewbot	July 5, 2013, 5:50 p.m.
Col: 80 E501 line too long (99 > 79 characters)	reviewbot	July 5, 2013, 5:50 p.m.
Col: 80 E501 line too long (102 > 79 characters)	reviewbot	July 5, 2013, 5:50 p.m.

reviewboard/hostingsvcs/github.py (Diff revision 1)

The issue has been resolved. Show all issues

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

reviewboard/hostingsvcs/github.py (Diff revision 1)
The issue has been resolved. Show all issues
```
No blank line.
```
reviewboard/hostingsvcs/github.py (Diff revision 1)
The issue has been resolved. Show all issues
```
No blank line.
```

reviewboard/hostingsvcs/github.py (Diff revision 1)

The issue has been dropped. 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.

david July 5, 2013, 2:54 p.m.

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

chipx86 July 5, 2013, 5:17 p.m.

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.

david July 5, 2013, 5:22 p.m.

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.

chipx86 July 5, 2013, 11:07 p.m.
```
Sounds fine then.
```

reviewboard/hostingsvcs/github.py (Diff revision 1)

The issue has been resolved. 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.

david July 5, 2013, 2:54 p.m.

I'm going to build on the "Commit" object.

reviewboard/hostingsvcs/tests.py (Diff revision 1)

The issue has been dropped. Show all issues

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

david July 5, 2013, 2:54 p.m.

I'll go through in a separate change and clean up all of these.

reviewboard/hostingsvcs/tests.py (Diff revision 1)

The issue has been resolved. 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.

reviewboard/hostingsvcs/tests.py (Diff revision 1)

The issue has been resolved. 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.

reviewboard/hostingsvcs/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
No blank line.

Same below.
```
reviewboard/scmtools/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
""" on the next line.
```
reviewboard/scmtools/svn.py (Diff revision 1)
The issue has been resolved. Show all issues
```
""" on the next line.
```

reviewboard/scmtools/svn.py (Diff revision 1)

The issue has been resolved. Show all issues

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

Diff:

Revision 2 (+328)

Show changes

	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/service.py
	reviewboard/hostingsvcs/tests.py
	reviewboard/scmtools/core.py
	reviewboard/scmtools/models.py
	reviewboard/scmtools/svn.py
	reviewboard/scmtools/tests.py

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:

reviewboard/hostingsvcs/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 32
 E201 whitespace after '{'
```
reviewboard/hostingsvcs/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 59
 E202 whitespace before '}'
```
reviewboard/hostingsvcs/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 35
 E201 whitespace after '{'
```
reviewboard/hostingsvcs/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 66
 E202 whitespace before '}'
```
reviewboard/hostingsvcs/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 29
 E201 whitespace after '['
```
reviewboard/hostingsvcs/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 31
 E201 whitespace after '{'
```
reviewboard/hostingsvcs/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 49
 E202 whitespace before '}'
```
reviewboard/hostingsvcs/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 51
 E202 whitespace before ']'
```
reviewboard/hostingsvcs/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 30
 E201 whitespace after '{'
```
reviewboard/hostingsvcs/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 46
 E202 whitespace before '}'
```
reviewboard/hostingsvcs/tests.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
reviewboard/hostingsvcs/tests.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
reviewboard/hostingsvcs/tests.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
reviewboard/hostingsvcs/tests.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
reviewboard/hostingsvcs/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 9
 E301 expected 1 blank line, found 0
```

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:

reviewboard/hostingsvcs/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
 local variable 'testcase' is assigned to but never used
```

Diff:

Revision 3 (+327)

Show changes

	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/service.py
	reviewboard/hostingsvcs/tests.py
	reviewboard/scmtools/core.py
	reviewboard/scmtools/models.py
	reviewboard/scmtools/svn.py
	reviewboard/scmtools/tests.py

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:

reviewboard/hostingsvcs/tests.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
reviewboard/hostingsvcs/tests.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
reviewboard/hostingsvcs/tests.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
reviewboard/hostingsvcs/tests.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```

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:

reviewboard/hostingsvcs/tests.py (Diff revision 3)

The issue has been resolved. 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.

david July 5, 2013, 5:48 p.m.

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.

Diff:

Revision 4 (+329)

Show changes

	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/service.py
	reviewboard/hostingsvcs/tests.py
	reviewboard/scmtools/core.py
	reviewboard/scmtools/models.py
	reviewboard/scmtools/svn.py
	reviewboard/scmtools/tests.py

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:

reviewboard/hostingsvcs/tests.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (99 > 79 characters)
```
reviewboard/hostingsvcs/tests.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (99 > 79 characters)
```
reviewboard/hostingsvcs/tests.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (102 > 79 characters)
```

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:

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to master (0516813).