[webkit-reviews] review denied: [Bug 47863] scm.py should be able tell us what revisions made changes to a given file : [Attachment 71101] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Adam Barth <abarth at webkit.org>'s
request for review:
Bug 47863: scm.py should be able tell us what revisions made changes to a given
file
https://bugs.webkit.org/show_bug.cgi?id=47863

Attachment 71101: Patch
https://bugs.webkit.org/attachment.cgi?id=71101&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=71101&action=review

Looks OK.  Testing could be better.

r- for no error-case testing.  Otherwise looks fine.

> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:249
> +	   self._subclass_must_implement()

using a helper function to raise confuses pylint.  I had it in an earlier
version of scm.py, but then moved to copy/paste exception throwing instead. 
Someone must have added it back.

> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:434
> +	   revisions = []

Any time you find yourself doing this, it may be cleaner as a
list-comprehension. :)

> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:437
> +	       match = re.search('^r(?P<revision>\d+) ', line)

If you worry this is hot we could store this in a class variable as a compiled
regexp.

> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:440
> +	       if not match:
> +		   continue
> +	       revisions.append(int(match.group('revision')))

So you coudl break this out into a self._revision_from_line(line) and then this
function is a simple list-comprehension. :)

I guess it would have to be
revisions = [self._revision_from_line(line) for line in lines]
return [revision for revision in revisions if revision]  # filter out None from
non-revision lines of output.

I'm not sure that's actually cleaner, but maybe I'm missing a slick way to do
the filtering.

> WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py:356
> +	   self.assertEqual(self.scm.revisions_changing_file("test_file"), [5,
4, 3, 2])

Are there other interesting cases to test?

What happens if you pass a non-existant file?

What about a file which hasnt' been changed in the latest revision?