[webkit-reviews] review denied: [Bug 33684] check-webkit-style: Provide a way to specify filter rules on a per-file/folder basis : [Attachment 48066] Proposed patch

[bugzilla-daemon at webkit.org]

Shinichiro Hamaji <hamaji at chromium.org> has denied Chris Jerdonek
<cjerdonek at webkit.org>'s request for review:
Bug 33684: check-webkit-style: Provide a way to specify filter rules on a
per-file/folder basis
https://bugs.webkit.org/show_bug.cgi?id=33684

Attachment 48066: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=48066&action=review

------- Additional Comments from Shinichiro Hamaji <hamaji at chromium.org>
Looks good in general. Especially I liked the cache structure of
FilterConfiguration as it looks efficient on both CPU and memory.

> +# The path-specific filter rules.
> +#
> +# Only the first substring match is used.  See the FilterConfiguration
> +# documentation for more information on this list.

I'd point the filename of FilterConfiguration. "See the FilterConfiguration
documentation in filter.py..." ?

> +	   if options.filter_configuration.user_rules:
> +	       # Only include the filter flag if user-provided rules are
present.
> +	       if options.filter_configuration.user_rules:
> +		   flags['filter'] = \
> +		       ",".join(options.filter_configuration.user_rules)

It seems we have a redundant if.

I don't like backslashes so much and I slightly prefer

user_rules = options.filter_configuration.user_rules
if user_rules:
    flags['filter'] = ",".join(user_rules)

But the current structure is fine as is.

> +	   for rule in user_rules:
> +	       if not (rule.startswith('+') or rule.startswith('-')):
> +		   raise ValueError('Invalid filter rule "%s": every rule '
> +				    'rule in the --filter flag must start '
> +				    'with + or -.' % rule)

I'm not sure why you moved this code chunk. As this is the check for filter
rules, having this check in filter.py seems OK. Could you explain?

> +	   self._base_rules = base_rules
> +	   self._path_specific = path_specific
> +	   # The "path_rules" values are tuples rather than lists so
> +	   # they can be used as dictionary keys.

I guess this comment should be in the docstring of path_specific or
above_path_rules_to_filter ?

> +	   self._path_rules_to_filter = {}
> +	   # Cached dictionary of path rules to CategoryFilter instance.

I think we usually put comments above the statement rather than below?

> +	   self._path_to_filter = {}
> +	   # Cached dictionary of file path to CategoryFilter instance.
> +	   #
> +	   # The same CategoryFilter instance can be shared across
> +	   # multiple keys. This allows us to take greater advantage of
> +	   # the caching done by CategoryFilter.should_check().

Ditto.

> +    def should_check(self, category, path):
> +	   """Return whether the given category should be checked."""
> +	   return self._filter_from_path(path).should_check(category)

Could you write a docstring for this function? This function is more important
than other functions as this is the interface of this class.

> +	   self.assertTrue(filter1.__eq__(filter2))
> +	   # Should not test with assertNotEqual.

Sorry, I cannot remember why we shouldn't. Could you add some more comments?
Also, this comment should be in above
"self.assertTrue(filter1.__eq__(filter2))" ?

> -	   # dynamic_cast is disallowed in most files.
> +	   # dynamic_cast is disallowed in most files .

We don't need the whitespace?

> -	   # It is explicitly allowed in tests, however.
> -	   self.assert_language_rules_check('foo_test.cpp', statement, '')
> -	   self.assert_language_rules_check('foo_unittest.cpp', statement, '')
> -	   self.assert_language_rules_check('foo_regtest.cpp', statement, '')
>  
>      # We cannot test this functionality because of difference of
>      # function definitions.	Anyway, we may never enable this.
> @@ -2056,16 +2052,6 @@ class OrderOfIncludesTest(CppStyleTestBase):
>					    '#include <assert.h>\n',
>					    '')
>  
> -    def test_webkit_api_test_excluded(self):
> -	   self.assert_language_rules_check('WebKitTools/WebKitAPITest/Test.h',

> -					    '#include "foo.h"\n',
> -					    '')
> -
> -    def test_webkit_api_test_excluded(self):
> -	   self.assert_language_rules_check('WebKit/qt/QGVLauncher/main.cpp',
> -					    '#include "foo.h"\n',
> -					    '')
> -
>      def test_check_line_break_after_own_header(self):
>	   self.assert_language_rules_check('foo.cpp',
>					    '#include "config.h"\n'

I want to keep these tests as regression tests. I guess you feel these tests
are redundant as these features are checked filter_unittest.py. But, in my
humble opinion, sometimes having duplicate test code is OK.