|Summary:||check-webkit-style: Refactor StyleChecker._process_file()|
|Product:||WebKit||Reporter:||Chris Jerdonek <cjerdonek>|
|Component:||Tools / Tests||Assignee:||Chris Jerdonek <cjerdonek>|
|Severity:||Normal||CC:||abarth, cjerdonek, hamaji, levin|
|Version:||528+ (Nightly build)|
Description Chris Jerdonek 2010-02-28 00:07:08 PST
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
Comment 1 Chris Jerdonek 2010-02-28 00:20:37 PST
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
Comment 2 Shinichiro Hamaji 2010-03-01 02:08:15 PST
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"], > + ) 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.
Comment 3 Chris Jerdonek 2010-03-01 22:40:26 PST
(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.