This is a follow-up to-- https://bugs.webkit.org/show_bug.cgi?id=34031 In that report, the filename parameter was removed from the style error handler function. That made it so many functions calls no longer use the filename parameter (so it no longer needs to be passed).
Created attachment 47606 [details] Proposed patch cq- since patch file might not apply correctly (since the ChangeLog portion of this git-generated patch file has leading context).
Comment on attachment 47606 [details] Proposed patch Looks good except the following nitpicks. > - if filename.endswith('.cpp'): > + if file_extension == ('cpp'): Unnecessary parentheses around 'cpp' ? > - elif filename.endswith('.c'): > + elif file_extension == ('c'): Ditto. > - error_message = include_state.check_next_include_order(header_type, filename.endswith('.h')) > + error_message = include_state.check_next_include_order(header_type, (file_extension == "h")) We can remove parentheses around 'file_extension == "h"', but please feel free not to modify this if you prefer this way or you know.
(In reply to comment #2) Thanks a lot for the quick review! > (From update of attachment 47606 [details]) > Looks good except the following nitpicks. > > > - if filename.endswith('.cpp'): > > + if file_extension == ('cpp'): > > Unnecessary parentheses around 'cpp' ? > > > - elif filename.endswith('.c'): > > + elif file_extension == ('c'): > > Ditto. It looks like the unit tests are not catching my carelessness here. I will investigate whether it is straightforward to get this coverage.
(In reply to comment #3) > > (From update of attachment 47606 [details] [details]) > > Looks good except the following nitpicks. > > > > > - if filename.endswith('.cpp'): > > > + if file_extension == ('cpp'): > > > > Unnecessary parentheses around 'cpp' ? > > > > > - elif filename.endswith('.c'): > > > + elif file_extension == ('c'): > > > > Ditto. > > It looks like the unit tests are not catching my carelessness here. I will > investigate whether it is straightforward to get this coverage. I checked and it looks like we do have unit test coverage here. It's just that ('c') is equivalent 'c'. (I didn't know off-hand.) It's only if there is a trailing comma that such notation is interpreted as a 1-tuple (in which case it would not be equivalent). See, for example-- http://docs.python.org/tutorial/datastructures.html#tuples-and-sequences It's some Python quirkiness that the documentation calls "ugly".
Manually committed (via "git svn dcommit"): http://trac.webkit.org/changeset/53998
> cq- since patch file might not apply correctly (since the ChangeLog portion of > this git-generated patch file has leading context). The commit queue is smart enough to deal with this case. It didn't used to be, but it is now.
Committed r54192: <http://trac.webkit.org/changeset/54192>
(In reply to comment #7) > Committed r54192: <http://trac.webkit.org/changeset/54192> Thanks a lot for the fix, Shinichiro. For what it's worth, this code path becomes unit-tested in the patch corresponding to this report: https://bugs.webkit.org/show_bug.cgi?id=34260 (in the processors.common.check_no_carriage_return() method).