Bug 35490 – check-webkit-style: Refactor StyleChecker._process_file()

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

[Chris Jerdonek]

The _process_file() method of webkitpy.style.StyleChecker currently does a few different things, including:

(1) Parsing the lines out of a file
(2) Logging if a file is not present
(3) Handling trailing carriage returns in lines.

Refactoring different file-processing functionality into appropriate methods will make it easier to incorporate other types of file-processing into check-webkit-style, for example processing licenses:

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

[Chris Jerdonek]

Created attachment 49695 [details]
Proposed patch

cq- since this patch depends on the patch for the following report being committed:

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

[Shinichiro Hamaji]

Comment on attachment 49695 [details]
Proposed patch

Looks good except for style nitpicks.

> +            self._handle_style_error(line_number + 1,  # Correct for offset.

I think we have only one space before a comment in new code (maybe because we are using this style in C++ code). Note that the code of style checker (especially cpp.py) may have 2 spaces because they are from Google.

> +        if error_lines:
> +            expected_errors = [(line_number, self._category,
> +                                self._confidence, self._expected_message)
> +                               for line_number in error_lines]
>              self.assertEquals(self._style_errors, expected_errors)
>          else:
>              self.assertEquals(self._style_errors, [])

I guess we don't need "if" and "else" here?

> +    def test_two_lines(self):
> +        # The CarriageReturnProcessor checks only the final character
> +        # of each line.
> +        self.assert_carriage_return(["line1", "line2\r"],
> +                                    ["line1", "line2"],
> +                                    [2])

What happens if we provide multiple '\r's ? I guess multiple error lines will appear because the mock error handler doesn't know about suppression? Anyway, I think it's worth adding a case for multiple '\r's.

[Chris Jerdonek]

(In reply to comment #2)
> (From update of attachment 49695 [details])
> Looks good except for style nitpicks.
> 
> > +            self._handle_style_error(line_number + 1,  # Correct for offset.
> 
> I think we have only one space before a comment in new code (maybe because we
> are using this style in C++ code). Note that the code of style checker
> (especially cpp.py) may have 2 spaces because they are from Google.

I did that because PEP8 requires at least two spaces for inline comments.

I will also address the other comments.  Good eye on the if-else.

[Chris Jerdonek]

Manually committed (via "git svn dcommit"):

http://trac.webkit.org/changeset/55411