Summary: | check-webkit-style: Refactor StyleChecker._process_file() | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Jerdonek <cjerdonek> | ||||
Component: | Tools / Tests | Assignee: | Chris Jerdonek <cjerdonek> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | abarth, cjerdonek, hamaji, levin | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Chris Jerdonek
2010-02-28 00:07:08 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 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. (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. Manually committed (via "git svn dcommit"): http://trac.webkit.org/changeset/55411 |