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

[bugzilla-daemon at webkit.org]

Shinichiro Hamaji <hamaji at chromium.org> has granted 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 48134: Proposed patch 2
https://bugs.webkit.org/attachment.cgi?id=48134&action=review

------- Additional Comments from Shinichiro Hamaji <hamaji at chromium.org>
Thanks for the update, especially for a lot of comments! This looks good.

> +def validate_filter_rules(filter_rules, all_categories):
> +    """Validate the given filter rules.
> +
> +    Args:
> +	 filter_rules: A list of boolean filter rules.
> +	 all_categories: A list of category names.

How about adding examples for these values?

By the way, thanks for adding this function. I think this solution is the best.


> +	   matches_some_category = False
> +	   for category in all_categories:
> +	       if category.startswith(rule[1:]):
> +		   matches_some_category = True
> +		   break
> +	   if not matches_some_category:
> +	       raise ValueError('Suspected incorrect filter rule "%s": '
> +				"the rule does not match the beginning "
> +				"of any category name." % rule)

Python has a weird syntax for this. You may be able to simplify this code like

	for category in all_categories:
	    if category.startswith(rule[1:]):
		break
	else:
	    raise ValueError('Suspected incorrect filter rule "%s": '
			     "the rule does not match the beginning "
			     "of any category name." % rule)

but yeah, this would look a bit strange. I'm not sure if we should encourage
the use of this feature, I personally like this though. Please use this syntax
if you don't hate this.

> In this case, processors/cpp_unittest.py doesn't have access to the
> PATH_RULES_SPECIFIER global variable, so we can't really keep these in the
same
> location anymore (without introducing a dependency I don't think we'd want to

> have).  What I did in the proposed patch was replace these tests with the
> following test code in checker_unittest.py:
> ...
> Is this acceptable to you? I could also add more pairs to test, but I'm not
> sure how valuable it would be since it essentially amounts to retyping
strings
> that appear in the PATH_RULES_SPECIFIER list and no additional code is
getting
> tested.  The one case it might be worthwhile is if you want to test the
> ordering of the elements, in case there are real-world paths that match more
> than one of the file patterns in the list.  But I don't know of any cases
like
> that with the current PATH_RULES_SPECIFIER.

I guess I'd just add the dependency (I don't care the dependency of unittests
so much), but of course, your solution is also good. Thanks for adding the
end-to-end test!

As for backslashes, actually, I found backslashes are used in PEP8 (and I was a
bit surprised because I'd rather use parentheses) when I commented on your
first patch. I'm not sure if we should always avoid backslashes. I asked you to
remove them for this time as it seemed we can easily remove them.

Thanks again!