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.
Created attachment 47802 [details] Proposed patch
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.
(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.
(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 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!
Comment on attachment 47802 [details] Proposed patch cq- to use webkit-patch.
Comment on attachment 47802 [details] Proposed patch Clearing flags on attachment: 47802 Committed r54126: <http://trac.webkit.org/changeset/54126>
All reviewed patches have been landed. Closing bug.