[SCM] WebKit Debian packaging branch, webkit-1.1, updated. upstream/1.1.20-204-g221d8e8
cjerdonek at webkit.org
cjerdonek at webkit.org
Wed Feb 10 22:17:02 UTC 2010
The following commit has been merged in the webkit-1.1 branch:
commit c01dc6f31181324a69fef288b1407b08794925a4
Author: cjerdonek at webkit.org <cjerdonek at webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Date: Fri Feb 5 20:44:45 2010 +0000
Provided a way in check-webkit-style to specify filter rules
on a per file or folder basis, via a configuration variable.
Reviewed by Shinichiro Hamaji.
https://bugs.webkit.org/show_bug.cgi?id=33684
* Scripts/webkitpy/style/checker.py:
- Added _PATH_RULES_SPECIFIER configuration variable.
- In ProcessorOptions class--
- Changed the CategoryFilter attribute to FilterConfiguration.
- Added path parameter to is_reportable().
- Renamed ArgumentDefaults filter_rules attribute to
base_filter_rules.
- Updated ArgumentPrinter class.
- Added filter rule validation to ArgumentParser (instead of
in CategoryFilter constructor).
* Scripts/webkitpy/style/checker_unittest.py:
- Updated unit tests as necessary.
- Added unit tests for PATH_RULES_SPECIFIER.
* Scripts/webkitpy/style/error_handlers.py:
- Updated DefaultStyleErrorHandler to use file path when
calling is_reportable().
* Scripts/webkitpy/style/error_handlers_unittest.py:
- Updated unit tests as necessary.
* Scripts/webkitpy/style/filter.py:
- Marked CategoryFilter internal with an underscore.
- Removed argument validation from CategoryFilter.
- Added FilterConfiguration class.
* Scripts/webkitpy/style/filter_unittest.py:
- Updated CategoryFilterTest class.
- Added FilterConfigurationTest unit tests.
* Scripts/webkitpy/style/processors/cpp.py:
- Removed _is_test_filename() code.
- Removed hard-coded path checks from check_include_line().
* Scripts/webkitpy/style/processors/cpp_unittest.py:
- Removed three unit tests related to exempted files.
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@54439 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
index 0a1f0f6..c5b63be 100644
--- a/WebKitTools/ChangeLog
+++ b/WebKitTools/ChangeLog
@@ -1,3 +1,50 @@
+2010-02-03 Chris Jerdonek <cjerdonek at webkit.org>
+
+ Reviewed by Shinichiro Hamaji.
+
+ Provided a way in check-webkit-style to specify filter rules
+ on a per file or folder basis, via a configuration variable.
+
+ https://bugs.webkit.org/show_bug.cgi?id=33684
+
+ * Scripts/webkitpy/style/checker.py:
+ - Added _PATH_RULES_SPECIFIER configuration variable.
+ - In ProcessorOptions class--
+ - Changed the CategoryFilter attribute to FilterConfiguration.
+ - Added path parameter to is_reportable().
+ - Renamed ArgumentDefaults filter_rules attribute to
+ base_filter_rules.
+ - Updated ArgumentPrinter class.
+ - Added filter rule validation to ArgumentParser (instead of
+ in CategoryFilter constructor).
+
+ * Scripts/webkitpy/style/checker_unittest.py:
+ - Updated unit tests as necessary.
+ - Added unit tests for PATH_RULES_SPECIFIER.
+
+ * Scripts/webkitpy/style/error_handlers.py:
+ - Updated DefaultStyleErrorHandler to use file path when
+ calling is_reportable().
+
+ * Scripts/webkitpy/style/error_handlers_unittest.py:
+ - Updated unit tests as necessary.
+
+ * Scripts/webkitpy/style/filter.py:
+ - Marked CategoryFilter internal with an underscore.
+ - Removed argument validation from CategoryFilter.
+ - Added FilterConfiguration class.
+
+ * Scripts/webkitpy/style/filter_unittest.py:
+ - Updated CategoryFilterTest class.
+ - Added FilterConfigurationTest unit tests.
+
+ * Scripts/webkitpy/style/processors/cpp.py:
+ - Removed _is_test_filename() code.
+ - Removed hard-coded path checks from check_include_line().
+
+ * Scripts/webkitpy/style/processors/cpp_unittest.py:
+ - Removed three unit tests related to exempted files.
+
2010-02-05 Kenneth Rohde Christiansen <kenneth at webkit.org>
Reviewed by Ariya Hidayat.
diff --git a/WebKitTools/Scripts/webkitpy/style/checker.py b/WebKitTools/Scripts/webkitpy/style/checker.py
index dc14ea3..81f10cf 100644
--- a/WebKitTools/Scripts/webkitpy/style/checker.py
+++ b/WebKitTools/Scripts/webkitpy/style/checker.py
@@ -37,7 +37,8 @@ import sys
from .. style_references import parse_patch
from error_handlers import DefaultStyleErrorHandler
from error_handlers import PatchStyleErrorHandler
-from filter import CategoryFilter
+from filter import validate_filter_rules
+from filter import FilterConfiguration
from processors.common import check_no_carriage_return
from processors.common import categories as CommonCategories
from processors.cpp import CppProcessor
@@ -85,6 +86,27 @@ WEBKIT_DEFAULT_FILTER_RULES = [
]
+# The path-specific filter rules.
+#
+# Only the first substring match is used. See the FilterConfiguration
+# documentation in filter.py for more information on this list.
+_PATH_RULES_SPECIFIER = [
+ # Files in these directories are consumers of the WebKit
+ # API and therefore do not follow the same header including
+ # discipline as WebCore.
+ (["WebKitTools/WebKitAPITest/",
+ "WebKit/qt/QGVLauncher/"],
+ ("-build/include",
+ "-readability/streams")),
+ # These are test file patterns.
+ (["_test.cpp",
+ "_unittest.cpp",
+ "_regtest.cpp"],
+ ("-readability/streams", # Many unit tests use cout.
+ "-runtime/rtti")),
+]
+
+
# Some files should be skipped when checking style. For example,
# WebKit maintains some files in Mozilla style on purpose to ease
# future merges.
@@ -201,43 +223,56 @@ Syntax: %(program_name)s [--verbose=#] [--git-commit=<SingleCommit>] [--output=v
return usage
+# FIXME: Eliminate support for "extra_flag_values".
+#
+# FIXME: Remove everything from ProcessorOptions except for the
+# information that can be passed via the command line, and
+# rename to something like CheckWebKitStyleOptions. This
+# includes, but is not limited to, removing the
+# max_reports_per_error attribute and the is_reportable()
+# method. See also the FIXME below to create a new class
+# called something like CheckerConfiguration.
+#
# This class should not have knowledge of the flag key names.
class ProcessorOptions(object):
- """A container to store options to use when checking style.
+ """A container to store options passed via the command line.
Attributes:
- output_format: A string that is the output format. The supported
- output formats are "emacs" which emacs can parse
- and "vs7" which Microsoft Visual Studio 7 can parse.
-
- verbosity: An integer between 1-5 inclusive that restricts output
- to errors with a confidence score at or above this value.
- The default is 1, which displays all errors.
+ extra_flag_values: A string-string dictionary of all flag key-value
+ pairs that are not otherwise represented by this
+ class. The default is the empty dictionary.
- filter: A CategoryFilter instance. The default is the empty filter,
- which means that all categories should be checked.
+ filter_configuration: A FilterConfiguration instance. The default
+ is the "empty" filter configuration, which
+ means that all errors should be checked.
git_commit: A string representing the git commit to check.
The default is None.
- extra_flag_values: A string-string dictionary of all flag key-value
- pairs that are not otherwise represented by this
- class. The default is the empty dictionary.
+ max_reports_per_error: The maximum number of errors to report
+ per file, per category.
- """
+ output_format: A string that is the output format. The supported
+ output formats are "emacs" which emacs can parse
+ and "vs7" which Microsoft Visual Studio 7 can parse.
+
+ verbosity: An integer between 1-5 inclusive that restricts output
+ to errors with a confidence score at or above this value.
+ The default is 1, which reports all errors.
+ """
def __init__(self,
- output_format="emacs",
- verbosity=1,
- filter=None,
- max_reports_per_category=None,
+ extra_flag_values=None,
+ filter_configuration = None,
git_commit=None,
- extra_flag_values=None):
+ max_reports_per_category=None,
+ output_format="emacs",
+ verbosity=1):
if extra_flag_values is None:
extra_flag_values = {}
- if filter is None:
- filter = CategoryFilter()
+ if filter_configuration is None:
+ filter_configuration = FilterConfiguration()
if max_reports_per_category is None:
max_reports_per_category = {}
@@ -252,7 +287,7 @@ class ProcessorOptions(object):
'Value given: "%s".' % verbosity)
self.extra_flag_values = extra_flag_values
- self.filter = filter
+ self.filter_configuration = filter_configuration
self.git_commit = git_commit
self.max_reports_per_category = max_reports_per_category
self.output_format = output_format
@@ -263,7 +298,7 @@ class ProcessorOptions(object):
"""Return whether this ProcessorOptions instance is equal to another."""
if self.extra_flag_values != other.extra_flag_values:
return False
- if self.filter != other.filter:
+ if self.filter_configuration != other.filter_configuration:
return False
if self.git_commit != other.git_commit:
return False
@@ -278,15 +313,16 @@ class ProcessorOptions(object):
# Useful for unit testing.
def __ne__(self, other):
- # Python does not automatically deduce from __eq__().
- return not (self == other)
+ # Python does not automatically deduce this from __eq__().
+ return not self.__eq__(other)
- def is_reportable(self, category, confidence_in_error):
+ def is_reportable(self, category, confidence_in_error, path):
"""Return whether an error is reportable.
An error is reportable if the confidence in the error
is at least the current verbosity level, and if the current
- filter says that the category should be checked.
+ filter says that the category should be checked for the
+ given path.
Args:
category: A string that is a style category.
@@ -294,15 +330,13 @@ class ProcessorOptions(object):
represents the application's confidence in
the error. A higher number signifies greater
confidence.
+ path: The path of the file being checked
"""
if confidence_in_error < self.verbosity:
return False
- if self.filter is None:
- return True # All categories should be checked by default.
-
- return self.filter.should_check(category)
+ return self.filter_configuration.should_check(category, path)
# This class should not have knowledge of the flag key names.
@@ -313,16 +347,16 @@ class ArgumentDefaults(object):
Attributes:
output_format: A string that is the default output format.
verbosity: An integer that is the default verbosity level.
- filter_rules: A list of strings that are boolean filter rules
- to prepend to any user-specified rules.
+ base_filter_rules: A list of strings that are boolean filter rules
+ to prepend to any user-specified rules.
"""
def __init__(self, default_output_format, default_verbosity,
- default_filter_rules):
+ default_base_filter_rules):
self.output_format = default_output_format
self.verbosity = default_verbosity
- self.filter_rules = default_filter_rules
+ self.base_filter_rules = default_base_filter_rules
class ArgumentPrinter(object):
@@ -345,11 +379,10 @@ class ArgumentPrinter(object):
flags['output'] = options.output_format
flags['verbose'] = options.verbosity
- if options.filter:
- # Only include the filter flag if rules are present.
- filter_string = str(options.filter)
- if filter_string:
- flags['filter'] = filter_string
+ # Only include the filter flag if user-provided rules are present.
+ user_rules = options.filter_configuration.user_rules
+ if user_rules:
+ flags['filter'] = ",".join(user_rules)
if options.git_commit:
flags['git-commit'] = options.git_commit
@@ -409,7 +442,7 @@ class ArgumentParser(object):
self.doc_print(' ' + category + '\n')
self.doc_print('\nDefault filter rules**:\n')
- for filter_rule in sorted(self.defaults.filter_rules):
+ for filter_rule in sorted(self.defaults.base_filter_rules):
self.doc_print(' ' + filter_rule + '\n')
self.doc_print('\n**The command always evaluates the above rules, '
'and before any --filter flag.\n\n')
@@ -417,10 +450,7 @@ class ArgumentParser(object):
sys.exit(0)
def _parse_filter_flag(self, flag_value):
- """Parse the value of the --filter flag.
-
- These filters are applied when deciding whether to emit a given
- error message.
+ """Parse the --filter flag, and return a list of filter rules.
Args:
flag_value: A string of comma-separated filter rules, for
@@ -457,7 +487,7 @@ class ArgumentParser(object):
output_format = self.defaults.output_format
verbosity = self.defaults.verbosity
- filter_rules = self.defaults.filter_rules
+ base_rules = self.defaults.base_filter_rules
# The flags already supported by the ProcessorOptions class.
flags = ['help', 'output=', 'verbose=', 'filter=', 'git-commit=']
@@ -480,6 +510,7 @@ class ArgumentParser(object):
extra_flag_values = {}
git_commit = None
+ user_rules = []
for (opt, val) in opts:
if opt == '--help':
@@ -494,7 +525,7 @@ class ArgumentParser(object):
if not val:
self._exit_with_categories()
# Prepend the defaults.
- filter_rules = filter_rules + self._parse_filter_flag(val)
+ user_rules = self._parse_filter_flag(val)
else:
extra_flag_values[opt] = val
@@ -508,15 +539,20 @@ class ArgumentParser(object):
'allowed output formats are emacs and vs7.' %
output_format)
+ all_categories = style_categories()
+ validate_filter_rules(user_rules, all_categories)
+
verbosity = int(verbosity)
if (verbosity < 1) or (verbosity > 5):
raise ValueError('Invalid --verbose value %s: value must '
'be between 1-5.' % verbosity)
- filter = CategoryFilter(filter_rules)
+ filter_configuration = FilterConfiguration(base_rules=base_rules,
+ path_specific=_PATH_RULES_SPECIFIER,
+ user_rules=user_rules)
options = ProcessorOptions(extra_flag_values=extra_flag_values,
- filter=filter,
+ filter_configuration=filter_configuration,
git_commit=git_commit,
max_reports_per_category=MAX_REPORTS_PER_CATEGORY,
output_format=output_format,
@@ -623,6 +659,18 @@ class ProcessorDispatcher(object):
return processor
+# FIXME: When creating the new CheckWebKitStyleOptions class as
+# described in a FIXME above, add a new class here called
+# something like CheckerConfiguration. The class should contain
+# attributes for options needed to process a file. This includes
+# a subset of the CheckWebKitStyleOptions attributes, a
+# FilterConfiguration attribute, an stderr_write attribute, a
+# max_reports_per_category attribute, etc. It can also include
+# the is_reportable() method. The StyleChecker should accept
+# an instance of this class rather than a ProcessorOptions
+# instance.
+
+
class StyleChecker(object):
"""Supports checking style in files and patches.
diff --git a/WebKitTools/Scripts/webkitpy/style/checker_unittest.py b/WebKitTools/Scripts/webkitpy/style/checker_unittest.py
index 814bd41..3bd2c1b 100755
--- a/WebKitTools/Scripts/webkitpy/style/checker_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/style/checker_unittest.py
@@ -37,10 +37,13 @@
import unittest
import checker as style
+from checker import _PATH_RULES_SPECIFIER as PATH_RULES_SPECIFIER
+from checker import style_categories
from checker import ProcessorDispatcher
from checker import ProcessorOptions
from checker import StyleChecker
-from filter import CategoryFilter
+from filter import validate_filter_rules
+from filter import FilterConfiguration
from processors.cpp import CppProcessor
from processors.text import TextProcessor
@@ -54,7 +57,7 @@ class ProcessorOptionsTest(unittest.TestCase):
# Check default parameters.
options = ProcessorOptions()
self.assertEquals(options.extra_flag_values, {})
- self.assertEquals(options.filter, CategoryFilter())
+ self.assertEquals(options.filter_configuration, FilterConfiguration())
self.assertEquals(options.git_commit, None)
self.assertEquals(options.max_reports_per_category, {})
self.assertEquals(options.output_format, "emacs")
@@ -70,14 +73,15 @@ class ProcessorOptionsTest(unittest.TestCase):
ProcessorOptions(verbosity=5) # works
# Check attributes.
+ filter_configuration = FilterConfiguration(base_rules=["+"])
options = ProcessorOptions(extra_flag_values={"extra_value" : 2},
- filter=CategoryFilter(["+"]),
+ filter_configuration=filter_configuration,
git_commit="commit",
max_reports_per_category={"category": 3},
output_format="vs7",
verbosity=3)
self.assertEquals(options.extra_flag_values, {"extra_value" : 2})
- self.assertEquals(options.filter, CategoryFilter(["+"]))
+ self.assertEquals(options.filter_configuration, filter_configuration)
self.assertEquals(options.git_commit, "commit")
self.assertEquals(options.max_reports_per_category, {"category": 3})
self.assertEquals(options.output_format, "vs7")
@@ -88,15 +92,18 @@ class ProcessorOptionsTest(unittest.TestCase):
# == calls __eq__.
self.assertTrue(ProcessorOptions() == ProcessorOptions())
- # Verify that a difference in any argument cause equality to fail.
+ # Verify that a difference in any argument causes equality to fail.
+ filter_configuration = FilterConfiguration(base_rules=["+"])
options = ProcessorOptions(extra_flag_values={"extra_value" : 1},
- filter=CategoryFilter(["+"]),
+ filter_configuration=filter_configuration,
git_commit="commit",
max_reports_per_category={"category": 3},
output_format="vs7",
verbosity=1)
self.assertFalse(options == ProcessorOptions(extra_flag_values={"extra_value" : 2}))
- self.assertFalse(options == ProcessorOptions(filter=CategoryFilter(["-"])))
+ new_config = FilterConfiguration(base_rules=["-"])
+ self.assertFalse(options ==
+ ProcessorOptions(filter_configuration=new_config))
self.assertFalse(options == ProcessorOptions(git_commit="commit2"))
self.assertFalse(options == ProcessorOptions(max_reports_per_category=
{"category": 2}))
@@ -113,36 +120,41 @@ class ProcessorOptionsTest(unittest.TestCase):
def test_is_reportable(self):
"""Test is_reportable()."""
- filter = CategoryFilter(["-xyz"])
- options = ProcessorOptions(filter=filter, verbosity=3)
+ filter_configuration = FilterConfiguration(base_rules=["-xyz"])
+ options = ProcessorOptions(filter_configuration=filter_configuration,
+ verbosity=3)
# Test verbosity
- self.assertTrue(options.is_reportable("abc", 3))
- self.assertFalse(options.is_reportable("abc", 2))
+ self.assertTrue(options.is_reportable("abc", 3, "foo.h"))
+ self.assertFalse(options.is_reportable("abc", 2, "foo.h"))
# Test filter
- self.assertTrue(options.is_reportable("xy", 3))
- self.assertFalse(options.is_reportable("xyz", 3))
+ self.assertTrue(options.is_reportable("xy", 3, "foo.h"))
+ self.assertFalse(options.is_reportable("xyz", 3, "foo.h"))
class GlobalVariablesTest(unittest.TestCase):
"""Tests validity of the global variables."""
+ def _all_categories(self):
+ return style.style_categories()
+
def defaults(self):
return style.webkit_argument_defaults()
def test_filter_rules(self):
defaults = self.defaults()
already_seen = []
- all_categories = style.style_categories()
- for rule in defaults.filter_rules:
+ validate_filter_rules(defaults.base_filter_rules,
+ self._all_categories())
+ # Also do some additional checks.
+ for rule in defaults.base_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 -.
self.assertTrue(rule.startswith('-'))
- self.assertTrue(rule[1:] in all_categories)
# Check no rule occurs twice.
self.assertFalse(rule in already_seen)
already_seen.append(rule)
@@ -158,11 +170,27 @@ class GlobalVariablesTest(unittest.TestCase):
# on valid arguments elsewhere.
parser.parse([]) # arguments valid: no error or SystemExit
+ def test_path_rules_specifier(self):
+ all_categories = style_categories()
+ for (sub_paths, path_rules) in PATH_RULES_SPECIFIER:
+ self.assertTrue(isinstance(path_rules, tuple),
+ "Checking: " + str(path_rules))
+ validate_filter_rules(path_rules, self._all_categories())
+
+ # 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/"))
+
def test_max_reports_per_category(self):
"""Check that MAX_REPORTS_PER_CATEGORY is valid."""
- categories = style.style_categories()
+ all_categories = self._all_categories()
for category in style.MAX_REPORTS_PER_CATEGORY.iterkeys():
- self.assertTrue(category in categories,
+ self.assertTrue(category in all_categories,
'Key "%s" is not a category' % category)
@@ -172,12 +200,15 @@ class ArgumentPrinterTest(unittest.TestCase):
_printer = style.ArgumentPrinter()
- def _create_options(self, output_format='emacs', verbosity=3,
- filter_rules=[], git_commit=None,
+ def _create_options(self,
+ output_format='emacs',
+ verbosity=3,
+ user_rules=[],
+ git_commit=None,
extra_flag_values={}):
- filter = CategoryFilter(filter_rules)
+ filter_configuration = FilterConfiguration(user_rules=user_rules)
return style.ProcessorOptions(extra_flag_values=extra_flag_values,
- filter=filter,
+ filter_configuration=filter_configuration,
git_commit=git_commit,
output_format=output_format,
verbosity=verbosity)
@@ -255,8 +286,8 @@ class ArgumentParserTest(unittest.TestCase):
parse(['--output=vs7']) # works
# Pass a filter rule not beginning with + or -.
- self.assertRaises(ValueError, parse, ['--filter=foo'])
- parse(['--filter=+foo']) # works
+ self.assertRaises(ValueError, parse, ['--filter=build'])
+ parse(['--filter=+build']) # works
# Pass files and git-commit at the same time.
self.assertRaises(SystemExit, parse, ['--git-commit=', 'file.txt'])
# Pass an extra flag already supported.
@@ -277,8 +308,9 @@ class ArgumentParserTest(unittest.TestCase):
self.assertEquals(options.output_format, 'vs7')
self.assertEquals(options.verbosity, 3)
- self.assertEquals(options.filter,
- CategoryFilter(["-", "+whitespace"]))
+ self.assertEquals(options.filter_configuration,
+ FilterConfiguration(base_rules=["-", "+whitespace"],
+ path_specific=PATH_RULES_SPECIFIER))
self.assertEquals(options.git_commit, None)
def test_parse_explicit_arguments(self):
@@ -291,13 +323,21 @@ class ArgumentParserTest(unittest.TestCase):
self.assertEquals(options.verbosity, 4)
(files, options) = parse(['--git-commit=commit'])
self.assertEquals(options.git_commit, 'commit')
- (files, options) = parse(['--filter=+foo,-bar'])
- self.assertEquals(options.filter,
- CategoryFilter(["-", "+whitespace", "+foo", "-bar"]))
- # Spurious white space in filter rules.
- (files, options) = parse(['--filter=+foo ,-bar'])
- self.assertEquals(options.filter,
- CategoryFilter(["-", "+whitespace", "+foo", "-bar"]))
+
+ # Pass user_rules.
+ (files, options) = parse(['--filter=+build,-whitespace'])
+ config = options.filter_configuration
+ self.assertEquals(options.filter_configuration,
+ FilterConfiguration(base_rules=["-", "+whitespace"],
+ path_specific=PATH_RULES_SPECIFIER,
+ user_rules=["+build", "-whitespace"]))
+
+ # Pass spurious white space in user rules.
+ (files, options) = parse(['--filter=+build, -whitespace'])
+ self.assertEquals(options.filter_configuration,
+ FilterConfiguration(base_rules=["-", "+whitespace"],
+ path_specific=PATH_RULES_SPECIFIER,
+ user_rules=["+build", "-whitespace"]))
# Pass extra flag values.
(files, options) = parse(['--extra'], ['extra'])
diff --git a/WebKitTools/Scripts/webkitpy/style/error_handlers.py b/WebKitTools/Scripts/webkitpy/style/error_handlers.py
index 31140de..1940e03 100644
--- a/WebKitTools/Scripts/webkitpy/style/error_handlers.py
+++ b/WebKitTools/Scripts/webkitpy/style/error_handlers.py
@@ -83,7 +83,7 @@ class DefaultStyleErrorHandler(object):
# A string to integer dictionary cache of the number of reportable
# errors per category passed to this instance.
- self._category_totals = { }
+ self._category_totals = {}
def _add_reportable_error(self, category):
"""Increment the error count and return the new category total."""
@@ -109,7 +109,9 @@ class DefaultStyleErrorHandler(object):
See the docstring of this module for more information.
"""
- if not self._options.is_reportable(category, confidence):
+ if not self._options.is_reportable(category,
+ confidence,
+ self._file_path):
return
category_total = self._add_reportable_error(category)
diff --git a/WebKitTools/Scripts/webkitpy/style/error_handlers_unittest.py b/WebKitTools/Scripts/webkitpy/style/error_handlers_unittest.py
index 83bdbb9..1d7e998 100644
--- a/WebKitTools/Scripts/webkitpy/style/error_handlers_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/style/error_handlers_unittest.py
@@ -48,11 +48,12 @@ class DefaultStyleErrorHandlerTest(StyleErrorHandlerTestBase):
"""Tests DefaultStyleErrorHandler class."""
+ _file_path = "foo.h"
+
_category = "whitespace/tab"
def _error_handler(self, options):
- file_path = "foo.h"
- return DefaultStyleErrorHandler(file_path,
+ return DefaultStyleErrorHandler(self._file_path,
options,
self._mock_increment_error_count,
self._mock_stderr_write)
@@ -81,7 +82,9 @@ class DefaultStyleErrorHandlerTest(StyleErrorHandlerTestBase):
self._check_initialized()
# Confirm the error is not reportable.
- self.assertFalse(options.is_reportable(self._category, confidence))
+ self.assertFalse(options.is_reportable(self._category,
+ confidence,
+ self._file_path))
self._call_error_handler(options, confidence)
@@ -147,9 +150,9 @@ class PatchStyleErrorHandlerTest(StyleErrorHandlerTestBase):
"""Tests PatchStyleErrorHandler class."""
- file_path = "__init__.py"
+ _file_path = "__init__.py"
- patch_string = """diff --git a/__init__.py b/__init__.py
+ _patch_string = """diff --git a/__init__.py b/__init__.py
index ef65bee..e3db70e 100644
--- a/__init__.py
+++ b/__init__.py
@@ -160,13 +163,13 @@ index ef65bee..e3db70e 100644
"""
def test_call(self):
- patch_files = parse_patch(self.patch_string)
- diff = patch_files[self.file_path]
+ patch_files = parse_patch(self._patch_string)
+ diff = patch_files[self._file_path]
options = ProcessorOptions(verbosity=3)
handle_error = PatchStyleErrorHandler(diff,
- self.file_path,
+ self._file_path,
options,
self._mock_increment_error_count,
self._mock_stderr_write)
@@ -176,7 +179,9 @@ index ef65bee..e3db70e 100644
message = "message"
# Confirm error is reportable.
- self.assertTrue(options.is_reportable(category, confidence))
+ self.assertTrue(options.is_reportable(category,
+ confidence,
+ self._file_path))
# Confirm error count initialized to zero.
self.assertEquals(0, self._error_count)
diff --git a/WebKitTools/Scripts/webkitpy/style/filter.py b/WebKitTools/Scripts/webkitpy/style/filter.py
index 1b41424..9a81c28 100644
--- a/WebKitTools/Scripts/webkitpy/style/filter.py
+++ b/WebKitTools/Scripts/webkitpy/style/filter.py
@@ -23,20 +23,47 @@
"""Contains filter-related code."""
-class CategoryFilter(object):
+def validate_filter_rules(filter_rules, all_categories):
+ """Validate the given filter rules, and raise a ValueError if not valid.
+
+ Args:
+ filter_rules: A list of boolean filter rules, for example--
+ ["-whitespace", "+whitespace/braces"]
+ all_categories: A list of all available category names, for example--
+ ["whitespace/tabs", "whitespace/braces"]
+
+ Raises:
+ ValueError: An error occurs if a filter rule does not begin
+ with "+" or "-" or if a filter rule does not match
+ the beginning of some category name in the list
+ of all available categories.
+
+ """
+ for rule in filter_rules:
+ if not (rule.startswith('+') or rule.startswith('-')):
+ raise ValueError('Invalid filter rule "%s": every rule '
+ "must start with + or -." % rule)
+
+ 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)
+
+
+class _CategoryFilter(object):
"""Filters whether to check style categories."""
def __init__(self, filter_rules=None):
"""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
+ symbol (+/-). The list should include any
default filter rules at the beginning.
Defaults to the empty list.
@@ -48,12 +75,6 @@ class CategoryFilter(object):
if filter_rules is None:
filter_rules = []
- 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
@@ -74,13 +95,13 @@ class CategoryFilter(object):
"""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.
+ 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
+ name. A plus (+) means the category should be checked, while a
minus (-) means the category should not be checked.
"""
@@ -95,3 +116,145 @@ class CategoryFilter(object):
self._should_check_category[category] = should_check # Update cache.
return should_check
+
+class FilterConfiguration(object):
+
+ """Supports filtering with path-specific and user-specified rules."""
+
+ def __init__(self, base_rules=None, path_specific=None, user_rules=None):
+ """Create a FilterConfiguration instance.
+
+ Args:
+ base_rules: The starting list of filter rules to use for
+ processing. The default is the empty list, which
+ by itself would mean that all categories should be
+ checked.
+
+ path_specific: A list of (sub_paths, path_rules) pairs
+ that stores the path-specific filter rules for
+ appending to the base rules.
+ The "sub_paths" value is a list of path
+ substrings. If a file path contains one of the
+ substrings, then the corresponding path rules
+ are appended. The first substring match takes
+ precedence, i.e. only the first match triggers
+ an append.
+ The "path_rules" value is the tuple of filter
+ rules that can be appended to the base rules.
+ The value is a tuple rather than a list so it
+ can be used as a dictionary key. The dictionary
+ is for caching purposes in the implementation of
+ this class.
+
+ user_rules: A list of filter rules that is always appended
+ to the base rules and any path rules. In other
+ words, the user rules take precedence over the
+ everything. In practice, the user rules are
+ provided by the user from the command line.
+
+ """
+ if base_rules is None:
+ base_rules = []
+ if path_specific is None:
+ path_specific = []
+ if user_rules is None:
+ user_rules = []
+
+ self._base_rules = base_rules
+ self._path_specific = path_specific
+
+ # FIXME: Make user rules internal after the FilterConfiguration
+ # attribute is removed from ProcessorOptions (since at
+ # that point ArgumentPrinter will no longer need to
+ # access FilterConfiguration.user_rules).
+ self.user_rules = user_rules
+
+ self._path_rules_to_filter = {}
+ """Cached dictionary of path rules to CategoryFilter instance."""
+
+ # The same CategoryFilter instance can be shared across
+ # multiple keys in this dictionary. This allows us to take
+ # greater advantage of the caching done by
+ # CategoryFilter.should_check().
+ self._path_to_filter = {}
+ """Cached dictionary of file path to CategoryFilter instance."""
+
+ # Useful for unit testing.
+ def __eq__(self, other):
+ """Return whether this FilterConfiguration is equal to another."""
+ if self._base_rules != other._base_rules:
+ return False
+ if self._path_specific != other._path_specific:
+ return False
+ if self.user_rules != other.user_rules:
+ return False
+
+ return True
+
+ # Useful for unit testing.
+ def __ne__(self, other):
+ # Python does not automatically deduce this from __eq__().
+ return not self.__eq__(other)
+
+ def _path_rules_from_path(self, path):
+ """Determine the path-specific rules to use, and return as a tuple."""
+ for (sub_paths, path_rules) in self._path_specific:
+ for sub_path in sub_paths:
+ if path.find(sub_path) > -1:
+ return path_rules
+ return () # Default to the empty tuple.
+
+ def _filter_from_path_rules(self, path_rules):
+ """Return the CategoryFilter associated to a path rules tuple."""
+ # We reuse the same CategoryFilter where possible to take
+ # advantage of the caching they do.
+ if path_rules not in self._path_rules_to_filter:
+ rules = list(self._base_rules) # Make a copy
+ rules.extend(path_rules)
+ rules.extend(self.user_rules)
+ self._path_rules_to_filter[path_rules] = _CategoryFilter(rules)
+
+ return self._path_rules_to_filter[path_rules]
+
+ def _filter_from_path(self, path):
+ """Return the CategoryFilter associated to a path."""
+ if path not in self._path_to_filter:
+ path_rules = self._path_rules_from_path(path)
+ filter = self._filter_from_path_rules(path_rules)
+ self._path_to_filter[path] = filter
+
+ return self._path_to_filter[path]
+
+ def should_check(self, category, path):
+ """Return whether the given category should be checked.
+
+ This method determines whether a category should be checked
+ by checking the category name against the filter rules for
+ the given path.
+
+ For a given path, the filter rules are the combination of
+ the base rules, the path-specific rules, and the user-provided
+ rules -- in that order. As we will describe below, later rules
+ in the list take precedence. The path-specific rules are the
+ rules corresponding to the first element of the "path_specific"
+ parameter that contains a string matching some substring of
+ the path. If there is no such element, there are no path-
+ specific rules for that path.
+
+ Given a list of filter rules, the logic for determining whether
+ a category should be checked is 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.
+
+ Args:
+ category: The category name.
+ path: The path of the file being checked.
+
+ """
+ return self._filter_from_path(path).should_check(category)
+
diff --git a/WebKitTools/Scripts/webkitpy/style/filter_unittest.py b/WebKitTools/Scripts/webkitpy/style/filter_unittest.py
index 0b12123..bdbff79 100644
--- a/WebKitTools/Scripts/webkitpy/style/filter_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/style/filter_unittest.py
@@ -22,10 +22,62 @@
"""Unit tests for filter.py."""
-
import unittest
-from filter import CategoryFilter
+from filter import _CategoryFilter as CategoryFilter
+from filter import validate_filter_rules
+from filter import FilterConfiguration
+
+# On Testing __eq__() and __ne__():
+#
+# In the tests below, we deliberately do not use assertEquals() or
+# assertNotEquals() to test __eq__() or __ne__(). We do this to be
+# very explicit about what we are testing, especially in the case
+# of assertNotEquals().
+#
+# Part of the reason is that it is not immediately clear what
+# expression the unittest module uses to assert "not equals" -- the
+# negation of __eq__() or __ne__(), which are not necessarily
+# equivalent expresions in Python. For example, from Python's "Data
+# Model" documentation--
+#
+# "There are no implied relationships among the comparison
+# operators. The truth of x==y does not imply that x!=y is
+# false. Accordingly, when defining __eq__(), one should
+# also define __ne__() so that the operators will behave as
+# expected."
+#
+# (from http://docs.python.org/reference/datamodel.html#object.__ne__ )
+
+class ValidateFilterRulesTest(unittest.TestCase):
+
+ """Tests validate_filter_rules() function."""
+
+ def test_validate_filter_rules(self):
+ all_categories = ["tabs", "whitespace", "build/include"]
+
+ bad_rules = [
+ "tabs",
+ "*tabs",
+ " tabs",
+ " +tabs",
+ "+whitespace/newline",
+ "+xxx",
+ ]
+
+ good_rules = [
+ "+tabs",
+ "-tabs",
+ "+build"
+ ]
+
+ for rule in bad_rules:
+ self.assertRaises(ValueError, validate_filter_rules,
+ [rule], all_categories)
+
+ for rule in good_rules:
+ # This works: no error.
+ validate_filter_rules([rule], all_categories)
class CategoryFilterTest(unittest.TestCase):
@@ -33,11 +85,15 @@ 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
+ """Test __init__ method."""
+ # Test that the attributes are getting set correctly.
+ filter = CategoryFilter(["+"])
+ self.assertEquals(["+"], filter._filter_rules)
+
+ def test_init_default_arguments(self):
+ """Test __init__ method default arguments."""
+ filter = CategoryFilter()
+ self.assertEquals([], filter._filter_rules)
def test_str(self):
"""Test __str__ "to string" operator."""
@@ -50,17 +106,20 @@ class CategoryFilterTest(unittest.TestCase):
filter2 = CategoryFilter(["+a", "+b"])
filter3 = CategoryFilter(["+b", "+a"])
- # == calls __eq__.
- self.assertTrue(filter1 == filter2)
- self.assertFalse(filter1 == filter3) # Cannot test with assertNotEqual.
+ # See the notes at the top of this module about testing
+ # __eq__() and __ne__().
+ self.assertTrue(filter1.__eq__(filter2))
+ self.assertFalse(filter1.__eq__(filter3))
def test_ne(self):
"""Test __ne__ inequality function."""
- # != calls __ne__.
# By default, __ne__ always returns true on different objects.
# Thus, just check the distinguishing case to verify that the
# code defines __ne__.
- self.assertFalse(CategoryFilter() != CategoryFilter())
+ #
+ # Also, see the notes at the top of this module about testing
+ # __eq__() and __ne__().
+ self.assertFalse(CategoryFilter().__ne__(CategoryFilter()))
def test_should_check(self):
"""Test should_check() method."""
@@ -82,3 +141,103 @@ class CategoryFilterTest(unittest.TestCase):
self.assertFalse(filter.should_check("abc"))
self.assertTrue(filter.should_check("a"))
+
+class FilterConfigurationTest(unittest.TestCase):
+
+ """Tests FilterConfiguration class."""
+
+ def _config(self, base_rules, path_specific, user_rules):
+ """Return a FilterConfiguration instance."""
+ return FilterConfiguration(base_rules=base_rules,
+ path_specific=path_specific,
+ user_rules=user_rules)
+
+ def test_init(self):
+ """Test __init__ method."""
+ # Test that the attributes are getting set correctly.
+ # We use parameter values that are different from the defaults.
+ base_rules = ["-"]
+ path_specific = [(["path"], ("+a",))]
+ user_rules = ["+"]
+
+ config = self._config(base_rules, path_specific, user_rules)
+
+ self.assertEquals(base_rules, config._base_rules)
+ self.assertEquals(path_specific, config._path_specific)
+ self.assertEquals(user_rules, config.user_rules)
+
+ def test_default_arguments(self):
+ # Test that the attributes are getting set correctly to the defaults.
+ config = FilterConfiguration()
+
+ self.assertEquals([], config._base_rules)
+ self.assertEquals([], config._path_specific)
+ self.assertEquals([], config.user_rules)
+
+ def test_eq(self):
+ """Test __eq__ method."""
+ # See the notes at the top of this module about testing
+ # __eq__() and __ne__().
+ self.assertTrue(FilterConfiguration().__eq__(FilterConfiguration()))
+
+ # Verify that a difference in any argument causes equality to fail.
+ config = FilterConfiguration()
+
+ # These parameter values are different from the defaults.
+ base_rules = ["-"]
+ path_specific = [(["path"], ("+a",))]
+ user_rules = ["+"]
+
+ self.assertFalse(config.__eq__(FilterConfiguration(
+ base_rules=base_rules)))
+ self.assertFalse(config.__eq__(FilterConfiguration(
+ path_specific=path_specific)))
+ self.assertFalse(config.__eq__(FilterConfiguration(
+ user_rules=user_rules)))
+
+ def test_ne(self):
+ """Test __ne__ method."""
+ # By default, __ne__ always returns true on different objects.
+ # Thus, just check the distinguishing case to verify that the
+ # code defines __ne__.
+ #
+ # Also, see the notes at the top of this module about testing
+ # __eq__() and __ne__().
+ self.assertFalse(FilterConfiguration().__ne__(FilterConfiguration()))
+
+ def test_base_rules(self):
+ """Test effect of base_rules on should_check()."""
+ base_rules = ["-b"]
+ path_specific = []
+ user_rules = []
+
+ config = self._config(base_rules, path_specific, user_rules)
+
+ self.assertTrue(config.should_check("a", "path"))
+ self.assertFalse(config.should_check("b", "path"))
+
+ def test_path_specific(self):
+ """Test effect of path_rules_specifier on should_check()."""
+ base_rules = ["-"]
+ path_specific = [(["path1"], ("+b",)),
+ (["path2"], ("+c",))]
+ user_rules = []
+
+ config = self._config(base_rules, path_specific, user_rules)
+
+ self.assertFalse(config.should_check("c", "path1"))
+ self.assertTrue(config.should_check("c", "path2"))
+ # Test that first match takes precedence.
+ self.assertFalse(config.should_check("c", "path2/path1"))
+
+ def test_user_rules(self):
+ """Test effect of user_rules on should_check()."""
+ base_rules = ["-"]
+ path_specific = []
+ user_rules = ["+b"]
+
+ config = self._config(base_rules, path_specific, user_rules)
+
+ self.assertFalse(config.should_check("a", "path"))
+ self.assertTrue(config.should_check("b", "path"))
+
diff --git a/WebKitTools/Scripts/webkitpy/style/processors/cpp.py b/WebKitTools/Scripts/webkitpy/style/processors/cpp.py
index e1f41a4..bbe5c8a 100644
--- a/WebKitTools/Scripts/webkitpy/style/processors/cpp.py
+++ b/WebKitTools/Scripts/webkitpy/style/processors/cpp.py
@@ -2030,22 +2030,6 @@ def _drop_common_suffixes(filename):
return os.path.splitext(filename)[0]
-def _is_test_filename(filename):
- """Determines if the given filename has a suffix that identifies it as a test.
-
- Args:
- filename: The input filename.
-
- Returns:
- True if 'filename' looks like a test, False otherwise.
- """
- if (filename.endswith('_test.cpp')
- or filename.endswith('_unittest.cpp')
- or filename.endswith('_regtest.cpp')):
- return True
- return False
-
-
def _classify_include(filename, include, is_system, include_state):
"""Figures out what kind of header 'include' is.
@@ -2110,7 +2094,6 @@ def _classify_include(filename, include, is_system, include_state):
return _OTHER_HEADER
-
def check_include_line(filename, file_extension, clean_lines, line_number, include_state, error):
"""Check rules that are applicable to #include lines.
@@ -2126,14 +2109,12 @@ def check_include_line(filename, file_extension, clean_lines, line_number, inclu
include_state: An _IncludeState instance in which the headers are inserted.
error: The function to call with any errors found.
"""
-
- 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
-
+ # FIXME: For readability or as a possible optimization, consider
+ # exiting early here by checking whether the "build/include"
+ # category should be checked for the given filename. This
+ # may involve having the error handler classes expose a
+ # should_check() method, in addition to the usual __call__
+ # method.
line = clean_lines.lines[line_number]
matched = _RE_PATTERN_INCLUDE.search(line)
@@ -2145,10 +2126,8 @@ def check_include_line(filename, file_extension, clean_lines, line_number, inclu
# Look for any of the stream classes that are part of standard C++.
if match(r'(f|ind|io|i|o|parse|pf|stdio|str|)?stream$', include):
- # 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.')
+ error(line_number, 'readability/streams', 3,
+ 'Streams are highly discouraged.')
# Look for specific includes to fix.
if include.startswith('wtf/') and not is_system:
@@ -2291,7 +2270,7 @@ def check_language(filename, clean_lines, line_number, file_extension, include_s
(matched.group(1), matched.group(2)))
# Check that we're not using RTTI outside of testing code.
- if search(r'\bdynamic_cast<', line) and not _is_test_filename(filename):
+ if search(r'\bdynamic_cast<', line):
error(line_number, 'runtime/rtti', 5,
'Do not use dynamic_cast<>. If you need to cast within a class '
"hierarchy, use static_cast<> to upcast. Google doesn't support "
diff --git a/WebKitTools/Scripts/webkitpy/style/processors/cpp_unittest.py b/WebKitTools/Scripts/webkitpy/style/processors/cpp_unittest.py
index e556cd3..8b1bf9c 100644
--- a/WebKitTools/Scripts/webkitpy/style/processors/cpp_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/style/processors/cpp_unittest.py
@@ -381,10 +381,6 @@ class CppStyleTest(CppStyleTestBase):
# dynamic_cast is disallowed in most files.
self.assert_language_rules_check('foo.cpp', statement, error_message)
self.assert_language_rules_check('foo.h', statement, error_message)
- # 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'
--
WebKit Debian packaging
More information about the Pkg-webkit-commits
mailing list