Bug 34932 – check-webkit-style: Code clean-up prior to dividing ProcessorOptions into two classes

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chris Jerdonek]

Splitting the patch of the below report into two, as requested:

https://bugs.webkit.org/show_bug.cgi?id=34674#c2

[Chris Jerdonek]

Created attachment 48829 [details]
Proposed patch

Note that I removed these patterns from _PATH_RULES_SPECIFIER:

-    # These are test file patterns.
-    (["_test.cpp",
-      "_unittest.cpp",
-      "_regtest.cpp"],
-     ("-readability/streams",  # Many unit tests use cout.
-      "-runtime/rtti")),

I found that no files in WebKit match them when looking for actual paths for the _PATH_RULES_SPECIFIER unit tests.  I recall Adam saying in a bugs.webkit.org comment that these were used previously by Google.  Thanks!

[Chris Jerdonek]

Created attachment 48830 [details]
Proposed patch 2

Minor correction to ChangeLog.

[Shinichiro Hamaji]

Comment on attachment 48830 [details]
Proposed patch 2

Looks good. Thanks a lot for isolating this change!

> +# Each string appearing in this nested list should have at least
> +# one associated unit test assertion.

I'd say where we should add the test. Maybe "See test_path_rules_specifier in checker_unittest.py" or something like this.

[Chris Jerdonek]

(In reply to comment #3)
> (From update of attachment 48830 [details])
> Looks good. Thanks a lot for isolating this change!

Sure thing.  Thanks for your quick review!

> > +# Each string appearing in this nested list should have at least
> > +# one associated unit test assertion.
> 
> I'd say where we should add the test. Maybe "See test_path_rules_specifier in
> checker_unittest.py" or something like this.

I was going to do this, but I decided against because comments like these can fall out of synch more easily.  I will add it back.  Thanks for the feedback.

[Chris Jerdonek]

Comment on attachment 48830 [details]
Proposed patch 2

cq- to commit manually.

[Chris Jerdonek]

Manually committed:

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