Bug 34249

Summary: check-webkit-style: Remove filename parameter from all functions where no longer used
Product: WebKit Reporter: Chris Jerdonek <cjerdonek>
Component: Tools / TestsAssignee: 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 Flags
Proposed patch hamaji: review+, cjerdonek: commit-queue-

Description Chris Jerdonek 2010-01-28 02:07:35 PST
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).
Comment 1 Chris Jerdonek 2010-01-28 04:31:55 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 2 Shinichiro Hamaji 2010-01-28 04:53:39 PST
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.
Comment 3 Chris Jerdonek 2010-01-28 04:58:37 PST
(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.
Comment 4 Chris Jerdonek 2010-01-28 05:15:35 PST
(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".
Comment 5 Chris Jerdonek 2010-01-28 05:23:46 PST
Manually committed (via "git svn dcommit"):

http://trac.webkit.org/changeset/53998
Comment 6 Adam Barth 2010-01-28 08:33:42 PST
> 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.
Comment 7 Shinichiro Hamaji 2010-02-01 18:45:53 PST
Committed r54192: <http://trac.webkit.org/changeset/54192>
Comment 8 Chris Jerdonek 2010-02-01 21:38:38 PST
(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).