Bug 33684

The correct fix to "overriding" the tab rule is to detect the presence of the svn allow-tabs property for the file. I don't think we should have a separate list of exclusions. We already have one way to exclude files from the no-tabs pre-commit hook, we should just re-use it here.

(In reply to comment #1) > The correct fix to "overriding" the tab rule is to detect the presence of the > svn allow-tabs property for the file. I don't think we should have a separate > list of exclusions. We already have one way to exclude files from the no-tabs > pre-commit hook, we should just re-use it here. Yeah, I had thought of that, too. I agree it's better in the case of tabs, but I thought we could start out with a short-term solution that could address potentially many birds. Are there are other cases where we want to specify rules on a per-directory basis? In terms of the correct fix for tabs, both the exclusion list approach and the allow-tabs approach have the disadvantage of exempting the entire file. In the case of the unit tests, we really only want to exclude the heredoc strings from the check (in the same way that Kenneth just wants "qt_" to be permitted and not all underscores). Do you think we shouldn't have an exclusion list for all rules, or just for the tab check? Note that the exclusion list would only affect the check for individual error categories and not exclude the entire file.

Regardless, then pre-commit hook works on a per-file basis. We could educate check-webkit-style to ignore tabs in heredoc strings, however it should *still* warn about those same files if they have tabs and don't have allow-tabs set. These seem like separate checks. The idea of excluding files/folders does seem potentially useful. However it might make more sense to include that information in the file itself with some sort of /* IGNORE STYLE */ comment or something.

(In reply to comment #3) > The idea of excluding files/folders does seem potentially useful. However it > might make more sense to include that information in the file itself with some > sort of /* IGNORE STYLE */ comment or something. I'm open to other approaches. I'll go ahead and change the title of the report to something less implementation-specific, and we can use this report to discuss possible solutions (and whether a general solution is even needed). I've also suggested the idea of using delineators to mark regions to skip over: https://bugs.webkit.org/show_bug.cgi?id=33098#c11 Some disadvantages of that approach are that-- (1) We have to touch our code. (2) Since the delineators will be spread all over our code base, it will be much harder to evolve the delineator convention if we want it to change. (3) It seems like it would be trickier to make the delineators specific to certain style categories (which would make (2) even more important). The primary advantage seems to be that it's more fine-grained than affecting the entire file.

> However it might make more sense to include that information in the file > itself with some sort of /* IGNORE STYLE */ comment or something. FYI, check-webkit-style has already had the NOLINT feature because Google's cpplint had it. If we decide to start using this feature, we may just need to change the keyword NOLINT as we eliminated the word "lint" from our style checkers.

(In reply to comment #1) > The correct fix to "overriding" the tab rule is to detect the presence of the > svn allow-tabs property for the file. I don't think we should have a separate > list of exclusions. We already have one way to exclude files from the no-tabs > pre-commit hook, we should just re-use it here. I filed a bug for this here: https://bugs.webkit.org/show_bug.cgi?id=33734

(In reply to comment #0) > (A better long-term solution would be for the file processor to ignore tabs > inside heredoc strings.) I also filed a bug report for this: https://bugs.webkit.org/show_bug.cgi?id=33737

I just noticed that some path-specific behavior already exists: def can_handle(filename): """Checks if this module supports the specified file type. Args: filename: A filename. It may contain directory names. """ return ("ChangeLog" in filename or "WebKitTools/Scripts/" in filename or os.path.splitext(filename)[1] in ('.css', '.html', '.idl', '.js', '.mm', '.php', '.pm', '.py', '.txt')) and not "LayoutTests/" in filename This is in text_style.py.

(In reply to comment #0) > > Were there other use cases? Here is another use-case for directory-specific exemptions from particular rules: https://bugs.webkit.org/show_bug.cgi?id=33356

It's looking like there are potentially many cases of this, so I will implement. Here is another one in the code: File names of the form *_test.cpp, *_unittest.cpp, and *_regtest.cpp are exempted from the "readability/streams" style check: > # Many unit tests use cout, so we exempt them. > if not _is_test_filename(filename): > error(line_number, 'readability/streams', 3, > 'Streams are highly discouraged.')

> File names of the form *_test.cpp, *_unittest.cpp, and *_regtest.cpp are > exempted from the "readability/streams" style check: This might be an artifact of the fact that this script comes from Google where streams are used extensively in unit tests. I'm not sure that exemption is meaningful here in WebKit-land.

(In reply to comment #11) > > File names of the form *_test.cpp, *_unittest.cpp, and *_regtest.cpp are > > exempted from the "readability/streams" style check: > > This might be an artifact of the fact that this script comes from Google where > streams are used extensively in unit tests. I'm not sure that exemption is > meaningful here in WebKit-land. Thanks for the info, Adam. There seem to be other cases as well. I didn't mention them all. For example, it looks like the [build/include_order] errors need to be suppressed for these specific folders: if (filename.find('WebKitTools/WebKitAPITest/') >= 0 or filename.find('WebKit/qt/QGVLauncher/') >= 0): # Files in this directory are consumers of the WebKit API and # therefore do not follow the same header including discipline as # WebCore. return

Yep. I think this would be a valuable facility to have.

Retitling (or changing back to something close to the original title, actually) to reflect the result of the discussion below and how this will be fixed by the assignee. We can always file a new report if we would like to enable NOLINT for code regions (using a word different from NOLINT), as was suggested in comment 3 and comment 5.

(In reply to comment #14) > We can always file a new report if we would like to enable NOLINT for code > regions (using a word different from NOLINT), as was suggested in comment 3 and > comment 5. I've been mildly quite but watching. I think NOLINT is a bit of a crutch and should be avoided if at all possible. It clutters the code makes things uglier. (I rather have a warning on a line rather than put in NOLINT).

(In reply to comment #15) > (In reply to comment #14) > > We can always file a new report if we would like to enable NOLINT for code > > regions (using a word different from NOLINT), as was suggested in comment 3 and > > comment 5. > > I've been mildly quite but watching. I think NOLINT is a bit of a crutch and > should be avoided if at all possible. It clutters the code makes things uglier. > (I rather have a warning on a line rather than put in NOLINT). Personally, I agree. I just didn't want the act of retitling to be interpreted as ending discussion on that approach (in case others feel a need for it).

Created attachment 48066 [details] Proposed patch Sorry this patch is on the bigger side. I'd like to break up ProcessorOptions into two classes next, as I've described in the FIXME. Also, I did a few scattered PEP8 touch-ups when I was near the code, but not systematically.

Comment on attachment 48066 [details] Proposed patch 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.

(In reply to comment #18) Thanks a lot for all of the valuable comments. Replies to some of your comments are below. > > +# 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..." ? This can also be obtained from the import statement, but sure. > > + 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) We should probably keep a list of things like this to consider for when we add Python style rules (e.g. "Do not use backslashes for line continuation."). (This one would be in addition to the PEP 8 rules it looks like we will be adopting.) Note that PEP 8 does include backslashes in their example illustrating where to break around binary operators. But I am okay with WebKit advising against. > > + 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? Sure. The immediate reason was because the CategoryFilter constructor no longer gets called right away with the new caching mechanism in FilterConfiguration. But more generally, I like giving the caller the choice as to whether or not they want validation logic to execute at runtime. One example is if the caller already knows a priori that the parameters are valid -- e.g. if they were validated by unit tests (as we are currently doing to some extent with the base portion of the rules and the path-specific rules). So I would rather expose a "validate" method or "construct with validation" method over putting it in the constructor. It also gives the caller more control as to what validation logic they want executed and what messages will display to the user in each case (without having to create new exception types for each type of error). For example, the caller may also want to validate that the string portion of each filter rule matches the beginning of some rule in the list of all available categories (we are also doing this with the base and path-specific portions of the rules). I will add a validate filter rules method to filter.py so the caller can instead call that -- since we don't need control over the error message text. > > + 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 ? Sure, I can move it. I originally put it as a code comment rather than a docstring since it seemed like more of an implementation comment rather than a usage comment. > > > + 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? Yes. This is partly a misreading on my part of PEP8/257, which says that attribute *docstrings* should appear below the simple assignment -- not comments. (I didn't know until recently that there even was such a thing as an "attribute docstring".) > > - # 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. Yes, I try to avoid test duplication if possible because it is easier to maintain. But I also recognize the value of end-to-end tests as sanity checks. 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: + def test_path_rules_specifier(self): + """Check PATH_RULES_SPECIFIER.""" + all_categories = style_categories() + for (sub_paths, path_rules) in PATH_RULES_SPECIFIER: + self.assertTrue(isinstance(path_rules, tuple), + "Checking: " + str(path_rules)) + for rule in path_rules: + self.assertTrue(rule[1:] in all_categories, + "Checking: " + rule) + + # Try using the path specifier (as an "end-to-end" check). + config = FilterConfiguration(path_specific=PATH_RULES_SPECIFIER) + self.assertTrue(config.should_check("xxx_any_category", + "xxx_non_matching_path")) + self.assertTrue(config.should_check("xxx_any_category", + "WebKitTools/WebKitAPITest/")) + self.assertFalse(config.should_check("build/include", + "WebKitTools/WebKitAPITest/")) 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. By the way, a couple things I noticed when removing those tests. They both have the same function name ("test_webkit_api_test_excluded"), so I'm not sure if they both get executed or not. Also, the 'WebKitTools/WebKitAPITest/Test.h' test continued to pass even after the refactoring (while 'WebKit/qt/QGVLauncher/main.cpp' did not). This turned out to be because > def check_next_include_order(self, header_type, file_is_header): was getting called with file_is_header True for the first one, so it doesn't look like that one was testing the exclusion after all.

Created attachment 48134 [details] Proposed patch 2

Comment on attachment 48134 [details] Proposed patch 2 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!

(In reply to comment #21) > (From update of attachment 48134 [details]) > > +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? Sure. I will document this better in general (e.g. add "Raises" section). :) > 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. Great -- thanks a lot for letting me know about this syntax. I am generally in favor of using any aspect of the language as long as it fits the purpose. > 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. I agree. Thanks again!

Manually committed (via "git svn dcommit") after incorporating Shinichiro's final suggestions: http://trac.webkit.org/changeset/54439

You don't like the "webkit-patch land --no-build" magic? ;)

(In reply to comment #24) > You don't like the "webkit-patch land --no-build" magic? ;) Oh, thanks for the tip! :) I had forgotten that webkit-patch has a "land" feature in addition to "land-from-bug". There are simply too many options for committing! :) Somewhere we should list the current "best" ways to commit in different scenarios (executable bit, moving files, etc) -- probably here: http://trac.webkit.org/wiki/CommitterTips Maybe I will organize that a bit better now that I have more experience with the different options (manual and automatic, Git and SVN).

(In reply to comment #24) > You don't like the "webkit-patch land --no-build" magic? ;) I started to try this here-- https://bugs.webkit.org/show_bug.cgi?id=34574#c6 but it seems like "webkit-patch land" doesn't support landing a git commit. Is that the case? The command "git svn dcommit" even detects the git commit for you. What would be cool is if webkit-patch could detect the git commit that hasn't been landed to the repository (like "git svn dcommit"), and also extract the bug number prior to updating the bugs database (e.g. from the git commit message or modified ChangeLog). In a lot of cases can't webkit-patch be getting the bug number from what is getting landed?