| Summary: | scm.py should be able tell us what revisions made changes to a given file | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||
| Component: | New Bugs | Assignee: | Adam Barth <abarth> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | commit-queue, eric | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Other | ||||||
| OS: | OS X 10.5 | ||||||
| Attachments: |
|
||||||
|
Description
Adam Barth
2010-10-18 16:57:05 PDT
Created attachment 71101 [details]
Patch
Comment on attachment 71101 [details] Patch 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? Comment on attachment 71101 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71101&action=review > WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py:357 > + self.assertRaises(ScriptError, self.scm.revisions_changing_file, "non_existent_file") Um. Clearly I'm blind. OK. Comment on attachment 71101 [details] Patch Clearing flags on attachment: 71101 Committed r70014: <http://trac.webkit.org/changeset/70014> All reviewed patches have been landed. Closing bug. |