Bug 107478 – Add functions to ChangeLog - parse bug desc/changed functions, delete/prepend entries

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Timothy Loh]

Add functions to ChangeLog - parse bug desc/changed functions, delete/prepend entries

[Timothy Loh]

Created attachment 183834 [details]
Patch

[WebKit Review Bot]

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.

[Timothy Loh]

Created attachment 183836 [details]
Patch

[Eric Seidel (no email)]

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.

[Timothy Loh]

(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.

[Timothy Loh]

Created attachment 183851 [details]
Patch

[Eric Seidel (no email)]

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.

[Timothy Loh]

Created attachment 184053 [details]
Patch

[WebKit Review Bot]

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

[Eric Seidel (no email)]

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?

[Eric Seidel (no email)]

(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.

[Timothy Loh]

(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.

[Eric Seidel (no email)]

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.

[WebKit Review Bot]

Comment on attachment 184053 [details]
Patch

Clearing flags on attachment: 184053

Committed r140491: <http://trac.webkit.org/changeset/140491>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.