Bug 107478 - Add functions to ChangeLog - parse bug desc/changed functions, delete/prepend entries
Summary: Add functions to ChangeLog - parse bug desc/changed functions, delete/prepend...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 74358
  Show dependency treegraph
 
Reported: 2013-01-21 14:59 PST by Timothy Loh
Modified: 2013-01-22 17:38 PST (History)
8 users (show)

See Also:


Attachments
Patch (12.29 KB, patch)
2013-01-21 15:00 PST, Timothy Loh
no flags Details | Formatted Diff | Diff
Patch (12.29 KB, patch)
2013-01-21 15:08 PST, Timothy Loh
no flags Details | Formatted Diff | Diff
Patch (16.55 KB, patch)
2013-01-21 16:21 PST, Timothy Loh
no flags Details | Formatted Diff | Diff
Patch (16.53 KB, patch)
2013-01-22 14:51 PST, Timothy Loh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Loh 2013-01-21 14:59:14 PST
Add functions to ChangeLog - parse bug desc/changed functions, delete/prepend entries
Comment 1 Timothy Loh 2013-01-21 15:00:44 PST
Created attachment 183834 [details]
Patch
Comment 2 WebKit Review Bot 2013-01-21 15:03:49 PST
Attachment 183834 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1
Tools/Scripts/webkitpy/common/checkout/changelog_unittest.py:478:  invalid syntax  [pylint/E0001] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Timothy Loh 2013-01-21 15:08:13 PST
Created attachment 183836 [details]
Patch
Comment 4 Eric Seidel (no email) 2013-01-21 15:54:45 PST
Comment on attachment 183836 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=183836&action=review

> Tools/Scripts/webkitpy/common/checkout/changelog.py:167
> +    @staticmethod
> +    def _parse_touched_functions(text):

I have (perhaps irational) fear of python's static methods.  They are hard to mock/use-mocks-from.  I'd rather these be at least class methods if not just plain old instance methods.

> Tools/Scripts/webkitpy/common/checkout/changelog.py:171
> +            file_match = re.match(ChangeLogEntry.touched_files_regexp, line)

can we call touched_files_regexp.match directly?  I thought the compiled regexp's had instance methods?  I guess the r'' syntax only gets you a regexp-aware string and not a compiled regexp?

> Tools/Scripts/webkitpy/common/checkout/changelog.py:198
> +        self._bug_description = ChangeLogEntry._parse_bug_description(self._contents)

self. will make these easier to mock/test.

> Tools/Scripts/webkitpy/common/checkout/changelog.py:209
> +        self._touched_functions = ChangeLogEntry._parse_touched_functions(self._contents)

same self.

> Tools/Scripts/webkitpy/common/checkout/changelog.py:259
> +    def bug_description(self):
> +        return self._bug_description

I never quite figured out pythons @property syntax, or if it was a good idea, so most of webkitpy uses getters like these.  I just need to figure out what official python style/best-practice is some day.
Comment 5 Timothy Loh 2013-01-21 16:15:38 PST
(In reply to comment #4)
> (From update of attachment 183836 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183836&action=review
> 
> > Tools/Scripts/webkitpy/common/checkout/changelog.py:167
> > +    @staticmethod
> > +    def _parse_touched_functions(text):
> 
> I have (perhaps irational) fear of python's static methods.  They are hard to mock/use-mocks-from.  I'd rather these be at least class methods if not just plain old instance methods.
> 
> > Tools/Scripts/webkitpy/common/checkout/changelog.py:198
> > +        self._bug_description = ChangeLogEntry._parse_bug_description(self._contents)
> 
> self. will make these easier to mock/test.
> 

Sure, I'll replace @staticmethod with @classmethod and ChangeLogEntry/ChangeLog with self throughout the file to make it uniform.

> > Tools/Scripts/webkitpy/common/checkout/changelog.py:171
> > +            file_match = re.match(ChangeLogEntry.touched_files_regexp, line)
> 
> can we call touched_files_regexp.match directly?  I thought the compiled regexp's had instance methods?  I guess the r'' syntax only gets you a regexp-aware string and not a compiled regexp?
> 

Yep, r'' stops backslashes from being escape characters.
Comment 6 Timothy Loh 2013-01-21 16:21:28 PST
Created attachment 183851 [details]
Patch
Comment 7 Eric Seidel (no email) 2013-01-22 14:41:33 PST
Comment on attachment 183851 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=183851&action=review

Aside from the self/cls confusion (which I likely lead you to), this looks fine.  You'll need to post the final patch with 'cls' replacing 'self' where necessary.

> Tools/Scripts/webkitpy/common/checkout/changelog.py:114
> +    def _parse_reviewer_text(self, text):

This is called "cls" when it's a class method.
Comment 8 Timothy Loh 2013-01-22 14:51:37 PST
Created attachment 184053 [details]
Patch
Comment 9 WebKit Review Bot 2013-01-22 15:34:16 PST
Comment on attachment 184053 [details]
Patch

Attachment 184053 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16067022

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
Comment 10 Eric Seidel (no email) 2013-01-22 16:29:51 PST
Comment on attachment 184053 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184053&action=review

> Tools/Scripts/webkitpy/common/checkout/changelog.py:450
> +        data = codecs.open(self.path, "r", "utf-8").read()
> +        codecs.open(self.path, "w", "utf-8").write(text + data)

Shouldn't we be using a FileSystem for this?  I'm sorry I didn't catch this before.  I guess ChangeLog doesn't correctly sit on top of our FileSystem abstraction anyway?
Comment 11 Eric Seidel (no email) 2013-01-22 16:30:08 PST
(In reply to comment #9)
> (From update of attachment 184053 [details])
> Attachment 184053 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/16067022
> 
> New failing tests:
> inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

Flaky tests are killing us.
Comment 12 Timothy Loh 2013-01-22 16:51:14 PST
(In reply to comment #10)
> (From update of attachment 184053 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184053&action=review
> 
> > Tools/Scripts/webkitpy/common/checkout/changelog.py:450
> > +        data = codecs.open(self.path, "r", "utf-8").read()
> > +        codecs.open(self.path, "w", "utf-8").write(text + data)
> 
> Shouldn't we be using a FileSystem for this?  I'm sorry I didn't catch this before.  I guess ChangeLog doesn't correctly sit on top of our FileSystem abstraction anyway?

ChangeLog uses the fileinput module (i.e. the real filesystem) throughout, so I thought it would be a bit inconsistent to add functions which use the FileSystem abstraction. Probably using FileSystem would be better though, so I'll see if getting rid of fileinput isn't too messy.
Comment 13 Eric Seidel (no email) 2013-01-22 16:58:39 PST
Comment on attachment 184053 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184053&action=review

>>> Tools/Scripts/webkitpy/common/checkout/changelog.py:450
>>> +        codecs.open(self.path, "w", "utf-8").write(text + data)
>> 
>> Shouldn't we be using a FileSystem for this?  I'm sorry I didn't catch this before.  I guess ChangeLog doesn't correctly sit on top of our FileSystem abstraction anyway?
> 
> ChangeLog uses the fileinput module (i.e. the real filesystem) throughout, so I thought it would be a bit inconsistent to add functions which use the FileSystem abstraction. Probably using FileSystem would be better though, so I'll see if getting rid of fileinput isn't too messy.

I agree.  This doesn't make ChangeLog any worse than it already is (in terms of mockability).  We can make ChagneLog FileSystem-compatible later in another bug.
Comment 14 WebKit Review Bot 2013-01-22 17:38:05 PST
Comment on attachment 184053 [details]
Patch

Clearing flags on attachment: 184053

Committed r140491: <http://trac.webkit.org/changeset/140491>
Comment 15 WebKit Review Bot 2013-01-22 17:38:10 PST
All reviewed patches have been landed.  Closing bug.