Bug 35490 - check-webkit-style: Refactor StyleChecker._process_file()
Summary: check-webkit-style: Refactor StyleChecker._process_file()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Chris Jerdonek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-28 00:07 PST by Chris Jerdonek
Modified: 2010-03-02 09:27 PST (History)
4 users (show)

See Also:


Attachments
Proposed patch (12.61 KB, patch)
2010-02-28 00:20 PST, Chris Jerdonek
hamaji: review+
cjerdonek: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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"],
> +                                    [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.
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.
Comment 4 Chris Jerdonek 2010-03-02 09:27:23 PST
Manually committed (via "git svn dcommit"):

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