Summary: | check-webkit-style: Separate the file-reading code from the rest of the style-checking code | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Jerdonek <cjerdonek> | ||||||
Component: | Tools / Tests | Assignee: | Chris Jerdonek <cjerdonek> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | cjerdonek, hamaji, levin, yael | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Bug Depends on: | 37754, 37850 | ||||||||
Bug Blocks: | |||||||||
Attachments: |
|
Description
Chris Jerdonek
2010-04-03 15:58:24 PDT
Created attachment 54244 [details] Proposed patch To make the review easier, I am doing the deletions of the old classes in a separate patch. Note also that because this patch eliminates the StyleChecker class, in this patch I am updating the TextFileReader class to use the file-existence check logic discussed and implemented in this patch: https://bugs.webkit.org/show_bug.cgi?id=37122 Comment on attachment 54244 [details]
Proposed patch
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?
(In reply to comment #2) > (From update of attachment 54244 [details]) Thanks for the prompt review! > WebKitTools/Scripts/webkitpy/style/checker.py:791 > + > Should we have a blank line here? Is this a recommended style I don't know? It's funny you ask. I originally didn't have it, but pep8.py flagged it. I agree it doesn't look right. Maybe I can add a comment so the reader of the code knows why it is there. > 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. Good point. It is probably in fact simply to check the messages here rather than discard them. That would be more appropriate for the end-to-end test. > 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? I will rename. Yes, the end-to-end test checks incrementing the error count. The code coverage test checks that the function is getting passed around properly inside the method, but doesn't actually call it. It was necessary for the caller to pass the error incrementer only so the caller could assert the equality of the DefaultStyleErrorHandler instances: + self.assertEquals(carriage_checker.style_error_handler, + expected_error_handler) + self.assertEquals(checker.style_error_handler, expected_error_handler) This capability was added recently here: https://bugs.webkit.org/show_bug.cgi?id=37850 (In reply to comment #3) > Good point. It is probably in fact simply to check the messages here rather > than discard them. That would be more appropriate for the end-to-end test. That should be-- "It is probably better in fact to simply check the messages here...." Created attachment 54275 [details]
Proposed patch 2
Addressing all comments.
Comment on attachment 54275 [details] Proposed patch 2 > It's funny you ask. I originally didn't have it, but pep8.py flagged it. I > agree it doesn't look right. Maybe I can add a comment so the reader of the > code knows why it is there. Hmm. So, let's leave the blank line. I'm not sure if we need the comment. If we put this comment here, we should put the same comment whenever we will write similar code. Or, maybe we can just create StyleProcessor.increment_error_count(). Anyway, please choose the way as you like. (In reply to comment #6) > Hmm. So, let's leave the blank line. I'm not sure if we need the comment. If we > put this comment here, we should put the same comment whenever we will write > similar code. In my experience, it's pretty rare for us to have a function definition inside the body of a method, so I would be okay with including the comment whenever the blank line would otherwise look awkward. (If we were going to include the blank line for spacing reasons anyways, then a comment would not be necessary.) I would characterize this as a bug in pep8.py since PEP8 only demands a blank line in between method definitions -- not before every function definition no matter where they appear: "Method definitions inside a class are separated by a single blank line." Comment on attachment 54275 [details] Proposed patch 2 Clearing flags on attachment: 54275 Committed r58249: <http://trac.webkit.org/changeset/58249> All reviewed patches have been landed. Closing bug. I think this patch is the cause to the style problem I am having in https://bugs.webkit.org/show_bug.cgi?id=38140. Any change to the file WebCore.vcproj will now cause a style error. (Even adding a space). (In reply to comment #10) > I think this patch is the cause to the style problem I am having in > https://bugs.webkit.org/show_bug.cgi?id=38140. > > Any change to the file WebCore.vcproj will now cause a style error. (Even > adding a space). This patch cleaned up some of the style-checking logic. What you're seeing is a consequence of the fact that .vcproj files were never listed as files that should be skipped: http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/style/checker.py?rev=58263#L177 This issue is being addressed here: https://bugs.webkit.org/show_bug.cgi?id=38149 (In reply to comment #11) > (In reply to comment #10) > > I think this patch is the cause to the style problem I am having in > > https://bugs.webkit.org/show_bug.cgi?id=38140. > > > > Any change to the file WebCore.vcproj will now cause a style error. (Even > > adding a space). > > This patch cleaned up some of the style-checking logic. What you're seeing is > a consequence of the fact that .vcproj files were never listed as files that > should be skipped: > > http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/style/checker.py?rev=58263#L177 > > This issue is being addressed here: > > https://bugs.webkit.org/show_bug.cgi?id=38149 Thank you for the explanation. |