2010-04-28 Chris Jerdonek <cjerdonek@webkit.org>
authoreric@webkit.org <eric@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 28 Apr 2010 11:19:41 +0000 (11:19 +0000)
committereric@webkit.org <eric@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 28 Apr 2010 11:19:41 +0000 (11:19 +0000)
        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: https://svn.webkit.org/repository/webkit/trunk@58401 268f45cc-cd09-0410-ab3c-d52691b4dbfc

WebKitTools/ChangeLog
WebKitTools/Scripts/webkitpy/style/checker.py
WebKitTools/Scripts/webkitpy/style/checker_unittest.py

index c032e60..5dd9d8d 100644 (file)
@@ -1,3 +1,32 @@
+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.
index 53aa827..3d5307a 100644 (file)
@@ -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__)
 
index 1d6d5b5..b9f9462 100755 (executable)
@@ -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):