Bug 33771 – style-check script reports loads of errors on gtk2drawing.c

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Gustavo Noronha (kov)]

Files WebCore/platform/gtk/gtk2drawing.c and WebCore/platform/gtk/gtk2drawing.h are special - they come from the Mozilla source tree, and we keep style modifications to a minimum on them - I would recommend simply ignoring them.

[Adam Barth]

Created attachment 46762 [details]
Patch

[Adam Barth]

*** Bug 33609 has been marked as a duplicate of this bug. ***

[Darin Adler]

Comment on attachment 46762 [details]
Patch

Looks fine, rs=me

[WebKit Commit Bot]

Comment on attachment 46762 [details]
Patch

Clearing flags on attachment: 46762

Committed r53383: <http://trac.webkit.org/changeset/53383>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Jerdonek]

With this fix, will the style bot still log a single error?

sys.stderr.write('Ignoring %s; This file is exempt from the style guide.\n' % filename)

With files to exempt, I just wanted to clarify whether they should be silently ignored, or if they should be skipped after logging a single line.

(Again, I'm not sure if the style bot is smart enough to distinguish between different types of errors logged to sys.stderr.write.)

[Adam Barth]

(In reply to comment #6)
> With this fix, will the style bot still log a single error?

The style bot is smart enough to know whether there were any errors.  It looks at the exit code to figure out if the check passed.

> With files to exempt, I just wanted to clarify whether they should be silently
> ignored, or if they should be skipped after logging a single line.

I put the logging in to make sure that folks using the tool know that they aren't getting coverage on those files.  It will show up in bugzilla if there are other errors caught by the bot, but it won't generate a bug post on its own.

[Chris Jerdonek]

(In reply to comment #7)
> (In reply to comment #6)
> > With this fix, will the style bot still log a single error?
> 
> The style bot is smart enough to know whether there were any errors.  It looks
> at the exit code to figure out if the check passed.
> 
> > With files to exempt, I just wanted to clarify whether they should be silently
> > ignored, or if they should be skipped after logging a single line.
> 
> I put the logging in to make sure that folks using the tool know that they
> aren't getting coverage on those files.  It will show up in bugzilla if there
> are other errors caught by the bot, but it won't generate a bug post on its
> own.

Okay, thanks, Adam.  Just to clarify, then, it seems like there are two classes of file exemptions:

(1) exemptions-with-warnings, e.g. files named "gtk2drawing.c"
    and files in the folder "WebKit/qt/Api/", and
(2) exemptions-without-warnings, e.g. all files in the "LayoutTests"
    folder.

If that's correct, I will make an explicit note of this in the code in my next patch.  Can you provide a rough rule of thumb as to which type new exemptions should belong to?  For example, which type should the "JavaScriptGlue" folder belong to?

https://lists.webkit.org/pipermail/webkit-dev/2009-December/011047.html

(e-mail referencing JavaScriptGlue)

[Adam Barth]

> Okay, thanks, Adam.  Just to clarify, then, it seems like there are two classes
> of file exemptions:
> 
> (1) exemptions-with-warnings, e.g. files named "gtk2drawing.c"
>     and files in the folder "WebKit/qt/Api/", and
> (2) exemptions-without-warnings, e.g. all files in the "LayoutTests"
>     folder.

Yes.  Although the code calls (2) can_handle.

> If that's correct, I will make an explicit note of this in the code in my next
> patch.  Can you provide a rough rule of thumb as to which type new exemptions
> should belong to?

It's some optimization problem between the principle of least surprise and not spamming useless information.  For example, I don't think anyone would be surprised that the LayoutTests aren't covered and if we included a warning that warning would be spammed on every patch.  On the other hand, it's kind of surprising that don't have style coverage for gtk2drawing.c and that file isn't modified that often.  That makes printing the error a better trade-off.

> For example, which type should the "JavaScriptGlue" folder
> belong to?

I'd print the warning.  I've never seen that folder changed so the spam problem is quite low.