Bug 34249 – check-webkit-style: Remove filename parameter from all functions where no longer used

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

[Chris Jerdonek]

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

[Chris Jerdonek]

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

[Shinichiro Hamaji]

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.

[Chris Jerdonek]

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

[Chris Jerdonek]

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

[Chris Jerdonek]

Manually committed (via "git svn dcommit"):

http://trac.webkit.org/changeset/53998

[Adam Barth]

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

[Shinichiro Hamaji]

Committed r54192: <http://trac.webkit.org/changeset/54192>

[Chris Jerdonek]

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