Bug 34379 – check-webkit-style: Break the style error handlers out into a separate class

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Chris Jerdonek]

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.

[Chris Jerdonek]

Created attachment 47802 [details]
Proposed patch

[Shinichiro Hamaji]

Comment on attachment 47802 [details]
Proposed patch

> -            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.

[Chris Jerdonek]

(In reply to comment #2)

Thanks for the quick review, Shinichiro!

> (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.

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.

[Chris Jerdonek]

(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.

[Shinichiro Hamaji]

Comment on attachment 47802 [details]
Proposed patch

> 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!

[Chris Jerdonek]

Comment on attachment 47802 [details]
Proposed patch

cq- to use webkit-patch.

[Chris Jerdonek]

Comment on attachment 47802 [details]
Proposed patch

Clearing flags on attachment: 47802

Committed r54126: <http://trac.webkit.org/changeset/54126>

[Chris Jerdonek]

All reviewed patches have been landed.  Closing bug.