Summary: | check-webkit-style: Remove filename parameter from all functions where no longer used | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Jerdonek <cjerdonek> | ||||
Component: | Tools / Tests | Assignee: | Chris Jerdonek <cjerdonek> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | abarth, cjerdonek, eric, hamaji | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Chris Jerdonek
2010-01-28 02:07:35 PST
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). |