Bug 34379 - check-webkit-style: Break the style error handlers out into a separate class
: check-webkit-style: Break the style error handlers out into a separate class
Status: RESOLVED FIXED
: WebKit
Tools / Tests
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
: 34260
  Show dependency treegraph
 
Reported: 2010-01-30 13:48 PST by
Modified: 2010-01-31 23:06 PST (History)


Attachments
Proposed patch (25.23 KB, patch)
2010-01-31 15:11 PST, Chris Jerdonek
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-01-30 13:48:21 PST
This is a precursor to--

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

and will also allow for unit testing of the patch style error handler.
------- Comment #1 From 2010-01-31 15:11:29 PST -------
Created an attachment (id=47802) [details]
Proposed patch
------- Comment #2 From 2010-01-31 22:05:15 PST -------
(From update of attachment 47802 [details])
> -            handle_style_error = self._default_style_error_handler(file_path)
> -
> +            handle_style_error = DefaultStyleErrorHandler(file_path,
> +                                     self.options,
> +                                     self._increment_error_count,
> +                                     self._stderr_write)

I'm not sure if we usually break lines in this way.

handle_style_error = DefaultStyleErrorHandler(file_path,
                                              self.options,
                                              self._increment_error_count,
                                              self._stderr_write)

or

handle_style_error = DefaultStyleErrorHandler(file_path, self.options, self._increment_error_count, self._stderr_write)

?

> +        if line_number not in self._get_line_numbers():
> +            # Then the error is not reportable.
> +            return
> +
> +        self._default_error_handler(line_number, category, confidence,
> +                                    message)
> +

Did we fix this FIXME?

# FIXME: Make sure errors having line number zero are
#        logged -- like carriage-return errors.
------- Comment #3 From 2010-01-31 22:28:36 PST -------
(In reply to comment #2)

Thanks for the quick review, Shinichiro!

> (From update of attachment 47802 [details] [details])
> > -            handle_style_error = self._default_style_error_handler(file_path)
> > -
> > +            handle_style_error = DefaultStyleErrorHandler(file_path,
> > +                                     self.options,
> > +                                     self._increment_error_count,
> > +                                     self._stderr_write)
> 
> I'm not sure if we usually break lines in this way.

No, we haven't been, but we recently started wanting to abide by the 79 character line limit. The first alternative below runs into that limit.

The only other option I thought might make sense is indenting four characters over from handle_style_error, but this didn't look as good as what I submitted (since the arguments with that indenting would be so far away from the right side).

Is your second alternative a single long line?  (I'm not 100% sure since Bugzilla might be throwing off your wrapping.)

> handle_style_error = DefaultStyleErrorHandler(file_path,
>                                               self.options,
>                                               self._increment_error_count,
>                                               self._stderr_write)
> 
> or
> 
> handle_style_error = DefaultStyleErrorHandler(file_path, self.options,
> self._increment_error_count, self._stderr_write)
> 
> ?
> 
> > +        if line_number not in self._get_line_numbers():
> > +            # Then the error is not reportable.
> > +            return
> > +
> > +        self._default_error_handler(line_number, category, confidence,
> > +                                    message)
> > +
> 
> Did we fix this FIXME?
> 
> # FIXME: Make sure errors having line number zero are
> #        logged -- like carriage-return errors.

No, but the same FIXME appears elsewhere (it wasn't clear to me at the time where the eventual fix would lie):

> if carriage_return_found and os.linesep != '\r\n':
>     # FIXME: Make sure this error also shows up when checking
>     #        patches, if appropriate.
>     #
>     # Use 0 for line_number since outputting only one error for
>     # potentially several lines.
>     handle_style_error(file_path, 0, 'whitespace/newline', 1,
>                        'One or more unexpected \\r (^M) found;'
>                        'better to use only a \\n')

I decided to keep just the FIXME near the actual carriage return check instead of having both.  I'll be addressing this FIXME shortly in bug 34260 which this report currently blocks.
------- Comment #4 From 2010-01-31 22:35:18 PST -------
(In reply to comment #2)
> > +        if line_number not in self._get_line_numbers():
> > +            # Then the error is not reportable.
> > +            return
> > +
> > +        self._default_error_handler(line_number, category, confidence,
> > +                                    message)
> > +
> 
> Did we fix this FIXME?
> 
> # FIXME: Make sure errors having line number zero are
> #        logged -- like carriage-return errors.

Also, I decided to fix this using an approach different from having a "line zero", so keeping a FIXME in this location made less sense.
------- Comment #5 From 2010-01-31 22:45:30 PST -------
(From update of attachment 47802 [details])
> No, we haven't been, but we recently started wanting to abide by the 79
> character line limit. The first alternative below runs into that limit.
> 
> The only other option I thought might make sense is indenting four characters
> over from handle_style_error, but this didn't look as good as what I submitted
> (since the arguments with that indenting would be so far away from the right
> side).

OK. So, I think we want a rule for line breaking. For now, let's go ahead with
current code.

> I decided to keep just the FIXME near the actual carriage return check instead
> of having both.  I'll be addressing this FIXME shortly in bug 34260 which this
> report currently blocks.

I see. Thanks for the clarification!
------- Comment #6 From 2010-01-31 23:04:16 PST -------
(From update of attachment 47802 [details])
cq- to use webkit-patch.
------- Comment #7 From 2010-01-31 23:06:17 PST -------
(From update of attachment 47802 [details])
Clearing flags on attachment: 47802

Committed r54126: <http://trac.webkit.org/changeset/54126>
------- Comment #8 From 2010-01-31 23:06:24 PST -------
All reviewed patches have been landed.  Closing bug.