Commit 4a831099 authored by eric@webkit.org's avatar eric@webkit.org
Browse files

2010-04-28 Chris Jerdonek <cjerdonek@webkit.org>

        Reviewed by Shinichiro Hamaji.

        Adjusted check-webkit-style so that files with file type NONE
        are automatically skipped without warning.

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

        This change simplifies configuring which files to skip.  It also
        addresses an issue whereby check-webkit-style was unintentionally
        checking .vcproj files for carriage returns.

        * Scripts/webkitpy/style/checker.py:
          - Moved the C++, Python, and text file extensions to new
            module-level configuration variables.
          - Removed .pyc from the _SKIPPED_FILES_WITHOUT_WARNING configuration
            variable.
          - Changed the numeric values of the FileType enum so that
            FileType.NONE evaluates to False.
          - For ProcessorDispatcher.should_skip_without_warning():
            - Changed the method to return True for FileType.NONE files.
            - Made ChangeLog files an exception to getting skipped.
          - Changed the StyleProcessor.process() method to raise an
            exception if given a file path that should not be processed.

        * Scripts/webkitpy/style/checker_unittest.py:
          - Updated the unit tests and added more test cases as necessary.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@58401 268f45cc-cd09-0410-ab3c-d52691b4dbfc
parent a363da2f
2010-04-28 Chris Jerdonek <cjerdonek@webkit.org>
Reviewed by Shinichiro Hamaji.
Adjusted check-webkit-style so that files with file type NONE
are automatically skipped without warning.
https://bugs.webkit.org/show_bug.cgi?id=38197
This change simplifies configuring which files to skip. It also
addresses an issue whereby check-webkit-style was unintentionally
checking .vcproj files for carriage returns.
* Scripts/webkitpy/style/checker.py:
- Moved the C++, Python, and text file extensions to new
module-level configuration variables.
- Removed .pyc from the _SKIPPED_FILES_WITHOUT_WARNING configuration
variable.
- Changed the numeric values of the FileType enum so that
FileType.NONE evaluates to False.
- For ProcessorDispatcher.should_skip_without_warning():
- Changed the method to return True for FileType.NONE files.
- Made ChangeLog files an exception to getting skipped.
- Changed the StyleProcessor.process() method to raise an
exception if given a file path that should not be processed.
* Scripts/webkitpy/style/checker_unittest.py:
- Updated the unit tests and added more test cases as necessary.
2010-04-28 Eric Seidel <eric@webkit.org>
 
Reviewed by Jeremy Orlow.
......@@ -157,11 +157,51 @@ _PATH_RULES_SPECIFIER = [
]
_CPP_FILE_EXTENSIONS = [
'c',
'cpp',
'h',
]
_PYTHON_FILE_EXTENSION = 'py'
# FIXME: Include 'vcproj' files as text files after creating a mechanism
# for exempting them from the carriage-return checker (since they
# are Windows-only files).
_TEXT_FILE_EXTENSIONS = [
'ac',
'cc',
'cgi',
'css',
'exp',
'flex',
'gyp',
'gypi',
'html',
'idl',
'in',
'js',
'mm',
'php',
'pl',
'pm',
'pri',
'pro',
'rb',
'sh',
'txt',
# 'vcproj', # See FIXME above.
'wm',
'xhtml',
'y',
]
# Files to skip that are less obvious.
#
# Some files should be skipped when checking style. For example,
# WebKit maintains some files in Mozilla style on purpose to ease
# future merges.
#
# Include a warning for skipped files that are less obvious.
_SKIPPED_FILES_WITH_WARNING = [
# The Qt API and tests do not follow WebKit style.
# They follow Qt style. :)
......@@ -174,11 +214,12 @@ _SKIPPED_FILES_WITH_WARNING = [
]
# Don't include a warning for skipped files that are more common
# and more obvious.
# Files to skip that are more common or obvious.
#
# This list should be in addition to files with FileType.NONE. Files
# with FileType.NONE are automatically skipped without warning.
_SKIPPED_FILES_WITHOUT_WARNING = [
"LayoutTests/",
".pyc",
]
......@@ -329,11 +370,11 @@ def configure_logging(stream, logger=None, is_verbose=False):
# Enum-like idiom
class FileType:
NONE = 1
NONE = 0 # FileType.NONE evaluates to False.
# Alphabetize remaining types
CPP = 2
PYTHON = 3
TEXT = 4
CPP = 1
PYTHON = 2
TEXT = 3
# FIXME: Rename this class to CheckerDispatcher, rename the style/processors/
......@@ -347,23 +388,6 @@ class ProcessorDispatcher(object):
"""Supports determining whether and how to check style, based on path."""
cpp_file_extensions = (
'c',
'cpp',
'h',
)
text_file_extensions = (
'css',
'html',
'idl',
'js',
'mm',
'php',
'pm',
'txt',
)
def _file_extension(self, file_path):
"""Return the file extension without the leading dot."""
return os.path.splitext(file_path)[1].lstrip(".")
......@@ -377,6 +401,16 @@ class ProcessorDispatcher(object):
def should_skip_without_warning(self, file_path):
"""Return whether the given file should be skipped without a warning."""
if not self._file_type(file_path): # FileType.NONE.
return True
# Since "LayoutTests" is in _SKIPPED_FILES_WITHOUT_WARNING, make
# an exception to prevent files like "LayoutTests/ChangeLog" and
# "LayoutTests/ChangeLog-2009-06-16" from being skipped.
#
# FIXME: Figure out a good way to avoid having to add special logic
# for this special case.
if os.path.basename(file_path).startswith('ChangeLog'):
return False
for skipped_file in _SKIPPED_FILES_WITHOUT_WARNING:
if file_path.find(skipped_file) >= 0:
return True
......@@ -386,7 +420,7 @@ class ProcessorDispatcher(object):
"""Return the file type corresponding to the given file."""
file_extension = self._file_extension(file_path)
if (file_extension in self.cpp_file_extensions) or (file_path == '-'):
if (file_extension in _CPP_FILE_EXTENSIONS) or (file_path == '-'):
# FIXME: Do something about the comment below and the issue it
# raises since cpp_style already relies on the extension.
#
......@@ -394,18 +428,13 @@ class ProcessorDispatcher(object):
# reading from stdin, cpp_style tests should not rely on
# the extension.
return FileType.CPP
elif file_extension == "py":
elif file_extension == _PYTHON_FILE_EXTENSION:
return FileType.PYTHON
elif ("ChangeLog" in file_path or
elif (os.path.basename(file_path).startswith('ChangeLog') or
(not file_extension and "WebKitTools/Scripts/" in file_path) or
file_extension in self.text_file_extensions):
file_extension in _TEXT_FILE_EXTENSIONS):
return FileType.TEXT
else:
# FIXME: If possible, change this method to default to
# returning FileType.TEXT. The should_process() method
# should really encapsulate which files not to check.
# Currently, "skip" logic is spread between both this
# method and should_process.
return FileType.NONE
def _create_processor(self, file_type, file_path, handle_style_error,
......@@ -623,7 +652,6 @@ class StyleProcessor(ProcessorBase):
def should_process(self, file_path):
"""Return whether the file should be checked for style."""
if self._dispatcher.should_skip_without_warning(file_path):
return False
if self._dispatcher.should_skip_with_warning(file_path):
......@@ -671,13 +699,7 @@ class StyleProcessor(ProcessorBase):
min_confidence)
if checker is None:
# FIXME: Should we really be skipping files that return True
# for should_process()? Perhaps this should be a
# warning or an exception so we can find out if
# should_process() is missing any files.
_log.debug('File not a recognized type to check. Skipping: "%s"'
% file_path)
return
raise AssertionError("File should not be checked: '%s'" % file_path)
_log.debug("Using class: " + checker.__class__.__name__)
......
......@@ -272,12 +272,13 @@ class ProcessorDispatcherSkipTest(unittest.TestCase):
"""Tests the "should skip" methods of the ProcessorDispatcher class."""
def setUp(self):
self._dispatcher = ProcessorDispatcher()
def test_should_skip_with_warning(self):
"""Test should_skip_with_warning()."""
dispatcher = ProcessorDispatcher()
# Check a non-skipped file.
self.assertFalse(dispatcher.should_skip_with_warning("foo.txt"))
self.assertFalse(self._dispatcher.should_skip_with_warning("foo.txt"))
# Check skipped files.
paths_to_skip = [
......@@ -292,25 +293,46 @@ class ProcessorDispatcherSkipTest(unittest.TestCase):
]
for path in paths_to_skip:
self.assertTrue(dispatcher.should_skip_with_warning(path),
self.assertTrue(self._dispatcher.should_skip_with_warning(path),
"Checking: " + path)
def test_should_skip_without_warning(self):
"""Test should_skip_without_warning()."""
dispatcher = ProcessorDispatcher()
# Check a non-skipped file.
self.assertFalse(dispatcher.should_skip_without_warning("foo.txt"))
# Check skipped files.
paths_to_skip = [
# LayoutTests folder
"LayoutTests/foo.txt",
]
def _assert_should_skip_without_warning(self, path, is_checker_none,
expected):
# Check the file type before asserting the return value.
checker = self._dispatcher.dispatch_processor(file_path=path,
handle_style_error=None,
min_confidence=3)
message = 'while checking: %s' % path
self.assertEquals(checker is None, is_checker_none, message)
self.assertEquals(self._dispatcher.should_skip_without_warning(path),
expected, message)
def test_should_skip_without_warning__true(self):
"""Test should_skip_without_warning() for True return values."""
# Check a file with NONE file type.
path = 'foo.asdf' # Non-sensical file extension.
self._assert_should_skip_without_warning(path,
is_checker_none=True,
expected=True)
# Check files with non-NONE file type. These examples must be
# drawn from the _SKIPPED_FILES_WITHOUT_WARNING configuration
# variable.
path = os.path.join('LayoutTests', 'foo.txt')
self._assert_should_skip_without_warning(path,
is_checker_none=False,
expected=True)
def test_should_skip_without_warning__false(self):
"""Test should_skip_without_warning() for False return values."""
paths = ['foo.txt',
os.path.join('LayoutTests', 'ChangeLog'),
]
for path in paths_to_skip:
self.assertTrue(dispatcher.should_skip_without_warning(path),
"Checking: " + path)
for path in paths:
self._assert_should_skip_without_warning(path,
is_checker_none=False,
expected=False)
class ProcessorDispatcherDispatchTest(unittest.TestCase):
......@@ -411,18 +433,34 @@ class ProcessorDispatcherDispatchTest(unittest.TestCase):
"""Test paths that should be checked as text."""
paths = [
"ChangeLog",
"ChangeLog-2009-06-16",
"foo.ac",
"foo.cc",
"foo.cgi",
"foo.css",
"foo.exp",
"foo.flex",
"foo.gyp",
"foo.gypi",
"foo.html",
"foo.idl",
"foo.in",
"foo.js",
"foo.mm",
"foo.php",
"foo.pl",
"foo.pm",
"foo.pri",
"foo.pro",
"foo.rb",
"foo.sh",
"foo.txt",
"FooChangeLog.bak",
"WebCore/ChangeLog",
"WebCore/inspector/front-end/inspector.js",
"WebKitTools/Scripts/check-webkit-style",
"foo.wm",
"foo.xhtml",
"foo.y",
os.path.join("WebCore", "ChangeLog"),
os.path.join("WebCore", "inspector", "front-end", "inspector.js"),
os.path.join("WebKitTools", "Scripts", "check-webkit-style"),
]
for path in paths:
......@@ -441,8 +479,10 @@ class ProcessorDispatcherDispatchTest(unittest.TestCase):
"""Test paths that have no file type.."""
paths = [
"Makefile",
"foo.asdf", # Non-sensical file extension.
"foo.png",
"foo.exe",
"foo.vcproj",
]
for path in paths:
......@@ -726,18 +766,10 @@ class StyleProcessor_CodeCoverageTest(LoggingTestCase):
def test_process__no_checker_dispatched(self):
"""Test the process() method for a path with no dispatched checker."""
self._processor.process(lines=['line1', 'line2'],
file_path='foo/do_not_process.txt',
line_numbers=[100])
# As a sanity check, check that the carriage-return checker was
# instantiated. (This code path was already checked in other test
# methods in this test case.)
carriage_checker = self.carriage_checker
self.assertEquals(carriage_checker.lines, ['line1', 'line2'])
# Check that the style checker was not dispatched.
self.assertTrue(self._mock_dispatcher.dispatched_checker is None)
path = os.path.join('foo', 'do_not_process.txt')
self.assertRaises(AssertionError, self._processor.process,
lines=['line1', 'line2'], file_path=path,
line_numbers=[100])
class PatchCheckerTest(unittest.TestCase):
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment