[webkit-reviews] review granted: [Bug 35490] check-webkit-style: Refactor StyleChecker._process_file() : [Attachment 49695] Proposed patch

[bugzilla-daemon at webkit.org]

Shinichiro Hamaji <hamaji at chromium.org> has granted Chris Jerdonek
<cjerdonek at webkit.org>'s request for review:
Bug 35490: check-webkit-style: Refactor StyleChecker._process_file()
https://bugs.webkit.org/show_bug.cgi?id=35490

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

------- Additional Comments from Shinichiro Hamaji <hamaji at chromium.org>
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.