Bug 37066 – check-webkit-style: Separate the file-reading code from the rest of the style-checking code

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Chris Jerdonek]

This report is to refactor check-webkit-style to allow other types of processing operations to be done on files, for example extracting license texts from files:

https://bugs.webkit.org/show_bug.cgi?id=35465

and perhaps cleaning up certain style issues (e.g. namespace indenting).

We can do this as follows:

Put all the generic file/directory reading code into a class called FileProcessor, and add a "processor" parameter to its "check_file/check_directory" methods (probably renaming these methods to process_file and process_directory).

Similarly, split off the style-related code from the StyleChecker class into a StyleProcessor class, and give this class a process(lines, file_path) method and an on_complete() method (for printing the total error count after processing several files).

[Chris Jerdonek]

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

[Shinichiro Hamaji]

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?

[Chris Jerdonek]

(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

[Chris Jerdonek]

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

[Chris Jerdonek]

Created attachment 54275 [details]
Proposed patch 2

Addressing all comments.

[Shinichiro Hamaji]

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.

[Chris Jerdonek]

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

[Chris Jerdonek]

Comment on attachment 54275 [details]
Proposed patch 2

Clearing flags on attachment: 54275

Committed r58249: <http://trac.webkit.org/changeset/58249>

[Chris Jerdonek]

All reviewed patches have been landed.  Closing bug.

[Yael]

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

[Chris Jerdonek]

(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

[Yael]

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