Changeset 53185 in webkit

Timestamp:

Jan 13, 2010 7:52:51 AM (14 years ago)

Author:

hamaji@chromium.org

Message:

2010-01-13 Chris Jerdonek <​chris.jerdonek@gmail.com>

Reviewed by Shinichiro Hamaji.

Created a CategoryFilter class to encapsulate the logic of
filter rules.

​https://bugs.webkit.org/show_bug.cgi?id=33454

• Scripts/webkitpy/style/checker.py:
  • Added CategoryFilter class.

• Scripts/webkitpy/style/checker_unittest.py:
  • Added CategoryFilter unit tests.

• Scripts/webkitpy/style/cpp_style.py:
  • Updated filter methods to use CategoryFilter.

• Scripts/webkitpy/style/cpp_style_unittest.py:
  • Updated references to filters.

Location:

trunk/WebKitTools

Files:

• ChangeLog (modified) (1 diff)
• Scripts/webkitpy/style/checker.py (modified) (7 diffs)
• Scripts/webkitpy/style/checker_unittest.py (modified) (5 diffs)
• Scripts/webkitpy/style/cpp_style.py (modified) (4 diffs)
• Scripts/webkitpy/style/cpp_style_unittest.py (modified) (4 diffs)

trunk/WebKitTools/ChangeLog

@@ +1 @@
+2010-01-13  Chris Jerdonek  <chris.jerdonek@gmail.com>
+
+        Reviewed by Shinichiro Hamaji.
+
+        Created a CategoryFilter class to encapsulate the logic of
+        filter rules.
+
+        https://bugs.webkit.org/show_bug.cgi?id=33454
+
+        * Scripts/webkitpy/style/checker.py:
+          - Added CategoryFilter class.
+
+        * Scripts/webkitpy/style/checker_unittest.py:
+          - Added CategoryFilter unit tests.
+
+        * Scripts/webkitpy/style/cpp_style.py:
+          - Updated filter methods to use CategoryFilter.
+
+        * Scripts/webkitpy/style/cpp_style_unittest.py:
+          - Updated references to filters.
+
 2010-01-12  Shinichiro Hamaji  <hamaji@chromium.org>
 

trunk/WebKitTools/Scripts/webkitpy/style/checker.py

@@ -231 +231 @@
 
 
+class CategoryFilter(object):
+
+    """Filters whether to check style categories."""
+
+    def __init__(self, filter_rules):
+        """Create a category filter.
+
+        This method performs argument validation but does not strip
+        leading or trailing white space.
+
+        Args:
+          filter_rules: A list of strings that are filter rules, which
+                        are strings beginning with the plus or minus
+                        symbol (+/-). The list should include any
+                        default filter rules at the beginning.
+
+        Raises:
+          ValueError: Invalid filter rule if a rule does not start with
+                      plus ("+") or minus ("-").
+
+        """
+        for rule in filter_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)
+
+        self._filter_rules = filter_rules
+        self._should_check_category = {} # Cached dictionary of category to True/False
+
+    def __str__(self):
+        return ",".join(self._filter_rules)
+
+    def __eq__(self, other):
+        # This is useful for unit testing.
+        # Two category filters are the same if and only if their
+        # constituent filter rules are the same.
+        return (str(self) == str(other))
+
+    def should_check(self, category):
+        """Return whether the category should be checked.
+
+        The rules for determining whether a category should be checked
+        are as follows. By default all categories should be checked.
+        Then apply the filter rules in order from first to last, with
+        later flags taking precedence.
+
+        A filter rule applies to a category if the string after the
+        leading plus/minus (+/-) matches the beginning of the category
+        name. A plus (+) means the category should be checked, while a
+        minus (-) means the category should not be checked.
+
+        """
+        if category in self._should_check_category:
+            return self._should_check_category[category]
+
+        should_check = True # All categories checked by default.
+        for rule in self._filter_rules:
+            if not category.startswith(rule[1:]):
+                continue
+            should_check = rule.startswith('+')
+        self._should_check_category[category] = should_check # Update cache.
+        return should_check
+
+
 # This class should not have knowledge of the flag key names.
 class ProcessorOptions(object):
@@ -245 +310 @@
                  The default is 1, which displays all errors.
 
-      filter_rules: A list of strings that are boolean filter rules used
-                    to determine whether a style category should be checked.
-                    Each string should start with + or -. An example
-                    string is "+whitespace/indent". The list includes any
-                    prepended default filter rules. The default is the
-                    empty list, which includes all categories.
+      filter: A CategoryFilter instance. The default is the empty filter,
+              which means that all categories should be checked.
 
       git_commit: A string representing the git commit to check.
@@ -260 +321 @@
     """
 
-    def __init__(self, output_format, verbosity=1, filter_rules=None,
+    def __init__(self, output_format, verbosity=1, filter=None,
                  git_commit=None, extra_flag_values=None):
-        if filter_rules is None:
-            filter_rules = []
+        if filter is None:
+            filter = CategoryFilter([])
         if extra_flag_values is None:
             extra_flag_values = {}
@@ -269 +330 @@
         self.output_format = output_format
         self.verbosity = verbosity
-        self.filter_rules = filter_rules
+        self.filter = filter
         self.git_commit = git_commit
         self.extra_flag_values = extra_flag_values
@@ -286 +347 @@
     cpp_style._set_output_format(options.output_format)
     cpp_style._set_verbose_level(options.verbosity)
-    cpp_style._set_filters(options.filter_rules)
+    cpp_style._set_filter(options.filter)
 
 
@@ -329 +390 @@
         flags['output'] = options.output_format
         flags['verbose'] = options.verbosity
-        if options.filter_rules:
-            flags['filter'] = ','.join(options.filter_rules)
+        if options.filter:
+            # Only include the filter flag if rules are present.
+            filter_string = str(options.filter)
+            if filter_string:
+                flags['filter'] = filter_string
         if options.git_commit:
             flags['git-commit'] = options.git_commit
@@ -492 +556 @@
                              'be between 1-5.' % verbosity)
 
-        for rule in filter_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)
-
-        options = ProcessorOptions(output_format, verbosity, filter_rules,
+        filter = CategoryFilter(filter_rules)
+
+        options = ProcessorOptions(output_format, verbosity, filter,
                                    git_commit, extra_flag_values)
 

trunk/WebKitTools/Scripts/webkitpy/style/checker_unittest.py

@@ -38 +38 @@
 
 import checker as style
+from checker import CategoryFilter
+
+
+class CategoryFilterTest(unittest.TestCase):
+
+    """Tests CategoryFilter class."""
+
+    def test_init(self):
+        """Test __init__ constructor."""
+        self.assertRaises(ValueError, CategoryFilter, ["no_prefix"])
+        CategoryFilter([]) # No ValueError: works
+        CategoryFilter(["+"]) # No ValueError: works
+        CategoryFilter(["-"]) # No ValueError: works
+
+    def test_str(self):
+        """Test __str__ "to string" operator."""
+        filter = CategoryFilter(["+a", "-b"])
+        self.assertEquals(str(filter), "+a,-b")
+
+    def test_eq(self):
+        """Test __eq__ equality operator."""
+        filter1 = CategoryFilter(["+a", "+b"])
+        filter2 = CategoryFilter(["+a", "+b"])
+        filter3 = CategoryFilter(["+b", "+a"])
+        self.assertEquals(filter1, filter2)
+        self.assertNotEquals(filter1, filter3)
+
+    def test_should_check(self):
+        """Test should_check() method."""
+        filter = CategoryFilter([])
+        self.assertTrue(filter.should_check("everything"))
+        # Check a second time to exercise cache.
+        self.assertTrue(filter.should_check("everything"))
+
+        filter = CategoryFilter(["-"])
+        self.assertFalse(filter.should_check("anything"))
+        # Check a second time to exercise cache.
+        self.assertFalse(filter.should_check("anything"))
+
+        filter = CategoryFilter(["-", "+ab"])
+        self.assertTrue(filter.should_check("abc"))
+        self.assertFalse(filter.should_check("a"))
+
+        filter = CategoryFilter(["+", "-ab"])
+        self.assertFalse(filter.should_check("abc"))
+        self.assertTrue(filter.should_check("a"))
 
 
@@ -47 +93 @@
         already_seen = []
         for rule in style.WEBKIT_FILTER_RULES:
+            # Check no leading or trailing white space.
+            self.assertEquals(rule, rule.strip())
             # All categories are on by default, so defaults should
             # begin with -.
@@ -76 +124 @@
                         filter_rules=[], git_commit=None,
                         extra_flag_values={}):
-        return style.ProcessorOptions(output_format, verbosity, filter_rules,
+        filter = CategoryFilter(filter_rules)
+        return style.ProcessorOptions(output_format, verbosity, filter,
                                       git_commit, extra_flag_values)
 
@@ -174 +223 @@
         self.assertEquals(options.output_format, 'vs7')
         self.assertEquals(options.verbosity, 3)
-        self.assertEquals(options.filter_rules, ['-', '+whitespace'])
+        self.assertEquals(options.filter,
+                          CategoryFilter(["-", "+whitespace"]))
         self.assertEquals(options.git_commit, None)
 
@@ -188 +238 @@
         self.assertEquals(options.git_commit, 'commit')
         (files, options) = parse(['--filter=+foo,-bar'])
-        self.assertEquals(options.filter_rules,
-                          ['-', '+whitespace', '+foo', '-bar'])
+        self.assertEquals(options.filter,
+                          CategoryFilter(["-", "+whitespace", "+foo", "-bar"]))
 
         # Pass extra flag values.

trunk/WebKitTools/Scripts/webkitpy/style/cpp_style.py

@@ -249 +249 @@
         self.verbose_level = 1  # global setting.
         self.error_count = 0    # global count of reported errors
-        # filters to apply when emitting error messages
-        self.filters = []
+        # filter to apply when emitting error messages
+        self.filter = None
 
         # output format:
@@ -267 +267 @@
         return last_verbose_level
 
-    def set_filters(self, filters):
-        """Sets the error-message filters.
-
-        These filters are applied when deciding whether to emit a given
-        error message.
+    def set_filter(self, filter):
+        """Sets the error-message filter.
 
         Args:
-          filters: A list of strings that are boolean filter rules used
-                   to determine whether a style category should be checked.
-                   Each string should start with + or -. An example
-                   string is "+whitespace/indent". The list includes any
-                   prepended default filter rules.
-
-        Raises:
-          ValueError: Not all filters started with '+' or '-'. For example,
-                      "-,+whitespace,-whitespace/indent,whitespace/badfilter"
+          filter: A CategoryFilter instance.
+
         """
-        self.filters = []
-        for filter in filters:
-            clean_filter = filter.strip()
-            if clean_filter:
-                self.filters.append(clean_filter)
-        for filter in self.filters:
-            if not (filter.startswith('+') or filter.startswith('-')):
-                raise ValueError('Every filter in --filter must start with '
-                                 '+ or - (%s does not)' % filter)
+        self.filter = filter
 
     def reset_error_count(self):
@@ -326 +308 @@
 
 
-def _filters():
-    """Returns the module's list of output filters, as a list."""
-    return _cpp_style_state.filters
-
-
-def _set_filters(filters):
-    """Sets the module's error-message filters.
-
-    These filters are applied when deciding whether to emit a given
+def _filter():
+    """Returns the module's CategoryFilter instance."""
+    return _cpp_style_state.filter
+
+
+def _set_filter(filter):
+    """Sets the module's error-message filter.
+
+    The filter is applied when deciding whether to emit a given
     error message.
 
     Args:
-      filters: A list of strings that are boolean filter rules used
-               to determine whether a style category should be checked.
-               Each string should start with + or -. An example
-               string is "+whitespace/indent". The list includes any
-               prepended default filter rules.
-    """
-    _cpp_style_state.set_filters(filters)
+      filter: A CategoryFilter instance.
+
+    """
+    _cpp_style_state.set_filter(filter)
 
 
@@ -506 +485 @@
         return False
 
-    is_filtered = False
-    for one_filter in _filters():
-        if one_filter.startswith('-'):
-            if category.startswith(one_filter[1:]):
-                is_filtered = True
-        elif one_filter.startswith('+'):
-            if category.startswith(one_filter[1:]):
-                is_filtered = False
-        else:
-            assert False  # should have been checked for in set_filter.
-    if is_filtered:
-        return False
-
-    return True
+    filter = _filter()
+
+    if filter is None:
+        return True # All categories should be checked by default.
+
+    return filter.should_check(category)
 
 

trunk/WebKitTools/Scripts/webkitpy/style/cpp_style_unittest.py

@@ -42 +42 @@
 import unittest
 import cpp_style
-# FIXME: Remove the need to import something from checker. See the
+# FIXME: Remove the need to import anything from checker. See the
 #        FIXME notes near the STYLE_CATEGORIES definition for a
 #        suggestion on how to best do this.
 from checker import STYLE_CATEGORIES
+from checker import CategoryFilter
 
 # This class works as an error collector and replaces cpp_style.Error
@@ -1572 +1573 @@
 
     def test_filter(self):
-        old_filters = cpp_style._cpp_style_state.filters
+        old_filter = cpp_style._cpp_style_state.filter
         try:
-            cpp_style._cpp_style_state.set_filters(['-', '+whitespace', '-whitespace/indent'])
+            cpp_style._cpp_style_state.set_filter(CategoryFilter(['-', '+whitespace', '-whitespace/indent']))
             self.assert_lint(
                 '// Hello there ',
@@ -1582 +1583 @@
             self.assert_lint(' weird opening space', '')
         finally:
-            cpp_style._cpp_style_state.filters = old_filters
+            cpp_style._cpp_style_state.filter = old_filter
 
     def test_filter_appending(self):
-        old_filters = cpp_style._cpp_style_state.filters
+        old_filter = cpp_style._cpp_style_state.filter
         try:
             # Reset filters
-            cpp_style._cpp_style_state.set_filters(['-whitespace'])
+            cpp_style._cpp_style_state.set_filter(CategoryFilter(['-whitespace']))
             self.assert_lint('// Hello there ', '')
-            cpp_style._cpp_style_state.set_filters(['-whitespace', '+whitespace/end_of_line'])
+            cpp_style._cpp_style_state.set_filter(CategoryFilter(['-whitespace', '+whitespace/end_of_line']))
             self.assert_lint(
                 '// Hello there ',
@@ -1597 +1598 @@
             self.assert_lint(' weird opening space', '')
         finally:
-            cpp_style._cpp_style_state.filters = old_filters
+            cpp_style._cpp_style_state.filter = old_filter
 
     def test_unnamed_namespaces_in_headers(self):