Python class for accessing Rietveld reviews.

Python class for accessing Rietveld reviews.

A simple python class for accessing Rietveld issues. It's using the Rietveld
interface to lazily pull down review meta data as requested by the application.

R=agable@chromium.org, iannucci@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216960

[Mattias Nissler (ping if slow), 2013-08-05 09:10:19]

Please review.

[Mattias Nissler (ping if slow), 2013-08-08 10:15:14]

Gentle ping

[iannucci, 2013-08-08 10:35:20]

lgtm % misc comments

https://codereview.chromium.org/20517002/diff/1/reviewbot/patching.py
File reviewbot/patching.py (right):

https://codereview.chromium.org/20517002/diff/1/reviewbot/patching.py#newcode35
reviewbot/patching.py:35: """, re.VERBOSE)
Scary :( I really wish we had a real, robust, tested diff parsing library.

May also want to provide some examples (or better, tests) and/or comments on
this guy.

https://codereview.chromium.org/20517002/diff/1/reviewbot/patching.py#newcode51
reviewbot/patching.py:51: A line number can be 0 if it doesn't exist in the
old/new file.
why not None? Unless lines start at 1?

https://codereview.chromium.org/20517002/diff/1/reviewbot/patching.py#newcode59
reviewbot/patching.py:59: in_prelude = False
how do we exit the prelude for a multi-chunk patch?

https://codereview.chromium.org/20517002/diff/1/reviewbot/review.py
File reviewbot/review.py (right):

https://codereview.chromium.org/20517002/diff/1/reviewbot/review.py#newcode20
reviewbot/review.py:20: @util.lazy_property
I'm assuming this is a memoizer?

https://codereview.chromium.org/20517002/diff/1/reviewbot/review.py#newcode68
reviewbot/review.py:68: return util.LazyDict(retrieve_patchset)
Does a LazyDict initialize values when you first access the key? If so, what
happens if you iterate over it or do membership lookups? (just curious)

[Mattias Nissler (ping if slow), 2013-08-08 13:04:13]

Thanks for the review. Please take another look at the code changes and the
added tests.

https://codereview.chromium.org/20517002/diff/1/reviewbot/patching.py
File reviewbot/patching.py (right):

https://codereview.chromium.org/20517002/diff/1/reviewbot/patching.py#newcode35
reviewbot/patching.py:35: """, re.VERBOSE)
On 2013/08/08 10:35:20, iannucci wrote:
> Scary :( I really wish we had a real, robust, tested diff parsing library.
> 
> May also want to provide some examples (or better, tests) and/or comments on
> this guy.

I'm with you on this, but the python standard library doesn't seem to have patch
parsing, and the few hits on the web that I found weren't much better than this.
The code here is stolen from rietveld and simplified to the bare essentials.
I've reworked the code a bit and added some tests.

https://codereview.chromium.org/20517002/diff/1/reviewbot/patching.py#newcode51
reviewbot/patching.py:51: A line number can be 0 if it doesn't exist in the
old/new file.
On 2013/08/08 10:35:20, iannucci wrote:
> why not None? Unless lines start at 1?

None is indeed better. Changed.

https://codereview.chromium.org/20517002/diff/1/reviewbot/patching.py#newcode59
reviewbot/patching.py:59: in_prelude = False
On 2013/08/08 10:35:20, iannucci wrote:
> how do we exit the prelude for a multi-chunk patch?

Hunks start with @@, so we reset.

Multi-file patches are not supported by this code. They're just concatenated, so
at some point, we'll see another prelude. I've now added code that raises an
error when this happens.

https://codereview.chromium.org/20517002/diff/1/reviewbot/review.py
File reviewbot/review.py (right):

https://codereview.chromium.org/20517002/diff/1/reviewbot/review.py#newcode20
reviewbot/review.py:20: @util.lazy_property
On 2013/08/08 10:35:20, iannucci wrote:
> I'm assuming this is a memoizer?

Correct.

https://codereview.chromium.org/20517002/diff/1/reviewbot/review.py#newcode68
reviewbot/review.py:68: return util.LazyDict(retrieve_patchset)
On 2013/08/08 10:35:20, iannucci wrote:
> Does a LazyDict initialize values when you first access the key? If so, what
> happens if you iterate over it or do membership lookups? (just curious)

Yes, it resolves the value on first access. Iteration and membership test are
not supported at this point.

[agable, 2013-08-08 15:51:50]

https://codereview.chromium.org/20517002/diff/8001/reviewbot/patching.py
File reviewbot/patching.py (right):

https://codereview.chromium.org/20517002/diff/8001/reviewbot/patching.py#newc...
reviewbot/patching.py:88: raise PatchParseError(line)
Wouldn't be hard to turn this code into a simple state-machine: not in a patch;
in a header; in a hunk. Defining the transition into a header
(line.startswith('---')) shouldn't be hard. Then you could handle multifile
patches just fine. Do you explicitly not want to handle multifile patches?

https://codereview.chromium.org/20517002/diff/8001/reviewbot/review.py
File reviewbot/review.py (right):

https://codereview.chromium.org/20517002/diff/8001/reviewbot/review.py#newcode43
reviewbot/review.py:43: def __init__(self, issue_id, rietveld):
ordering of arguments opposite from above.

[Mattias Nissler (ping if slow), 2013-08-09 15:13:33]

Comments addressed. Good to go?

https://codereview.chromium.org/20517002/diff/8001/reviewbot/patching.py
File reviewbot/patching.py (right):

https://codereview.chromium.org/20517002/diff/8001/reviewbot/patching.py#newc...
reviewbot/patching.py:88: raise PatchParseError(line)
On 2013/08/08 15:51:50, Aaron Gable wrote:
> Wouldn't be hard to turn this code into a simple state-machine: not in a
patch;
> in a header; in a hunk. Defining the transition into a header
> (line.startswith('---')) shouldn't be hard. Then you could handle multifile
> patches just fine. Do you explicitly not want to handle multifile patches?

You are right, adding this wouldn't be hard. I don't have a use case for
multi-file patch files right now though, and the return value would also be more
complicated, so I'll leave this as is for now.

https://codereview.chromium.org/20517002/diff/8001/reviewbot/review.py
File reviewbot/review.py (right):

https://codereview.chromium.org/20517002/diff/8001/reviewbot/review.py#newcode43
reviewbot/review.py:43: def __init__(self, issue_id, rietveld):
On 2013/08/08 15:51:50, Aaron Gable wrote:
> ordering of arguments opposite from above.

Fixed.

[agable, 2013-08-09 20:50:52]

LGTM

[Mattias Nissler (ping if slow), 2013-08-12 08:25:40]

Committed patchset #3 manually as r216960 (presubmit successful).