[webkit-reviews] review granted: [Bug 37066] check-webkit-style: Separate the file-reading code from the rest of the style-checking code : [Attachment 54244] Proposed patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Apr 26 00:12:40 PDT 2010


Shinichiro Hamaji <hamaji at chromium.org> has granted Chris Jerdonek
<cjerdonek at webkit.org>'s request for review:
Bug 37066: check-webkit-style: Separate the file-reading code from the rest of
the style-checking code
https://bugs.webkit.org/show_bug.cgi?id=37066

Attachment 54244: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=54244&action=review

------- Additional Comments from Shinichiro Hamaji <hamaji at chromium.org>
Looks good! I put a few style nitpicks. All of them aren't serious, so please
feel free to ignore some of (or all of) them.

WebKitTools/Scripts/webkitpy/style/checker.py:791
 +  
Should we have a blank line here? Is this a recommended style I don't know?

WebKitTools/Scripts/webkitpy/style/checker_unittest.py:534
 +  
I think we can remove this blank line. This is inconsistent with test_process.

WebKitTools/Scripts/webkitpy/style/checker_unittest.py:524
 +	    pass
We may want to have a simple comment to mention we are just discarding stderr.
Or, we may be able to choose better name for this. Maybe discard_message or
something like this. In this way, I think the intention of its callers can be
slightly clearer.

WebKitTools/Scripts/webkitpy/style/checker_unittest.py:625
 +	def _mock_increment_error_count(self):
I would call this as do_nothing or something. Or, is it worth checking the
count of errors, StyleProcessor_EndToEndTest is also checking this though?


More information about the webkit-reviews mailing list