Attachment 49687 Details for Bug 35484 – Proposed patch

[patch] Proposed patch

_patch-35484-1.diff (text/plain), 14.11 KB, created by Chris Jerdonek on 2010-02-27 15:41:19 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Jerdonek

Created: 2010-02-27 15:41:19 PST

Size: 14.11 KB

patch

obsolete

>diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
>index de8c3c5..27b777c 100644
>--- a/WebKitTools/ChangeLog
>+++ b/WebKitTools/ChangeLog
>@@ -1,5 +1,34 @@
> 2010-02-27  Chris Jerdonek  <cjerdonek@webkit.org>
> 
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Started using the logging module in check-webkit-style.
>+        This provides more debugging options and a more flexible,
>+        uniform way to report messages to the end-user.
>+
>+        https://bugs.webkit.org/show_bug.cgi?id=35484
>+
>+        * Scripts/check-webkit-style:
>+          - Added a call to configure_logging() in the beginning of main().
>+          - Replaced two calls to sys.stderr.write() with appropriate
>+            logging calls.
>+
>+        * Scripts/webkitpy/style/checker.py:
>+          - Added a configure_logging() method.
>+          - Added a _LevelLoggingFilter class to filter out log messages
>+            above a certain logging level.
>+          - Removed the _stderr_write() method from the StyleChecker class
>+            and replaced its use with appropriate logging calls.
>+
>+        * Scripts/webkitpy/style/checker_unittest.py:
>+          - Added a CheckerTestBase class with setUp, tearDown, and other
>+            methods to facilitate the unit testing of log messages.
>+          - Added the ConfigureLoggingTest class to unit test the
>+            configure_logging() method.
>+          - Updated the StyleCheckerCheckFileTest class as necessary.
>+
>+2010-02-27  Chris Jerdonek  <cjerdonek@webkit.org>
>+
>         Reviewed by David Levin.
> 
>         Added Python style checking to check-webkit-style using
>diff --git a/WebKitTools/Scripts/check-webkit-style b/WebKitTools/Scripts/check-webkit-style
>index ea2e943..4d79be3 100755
>--- a/WebKitTools/Scripts/check-webkit-style
>+++ b/WebKitTools/Scripts/check-webkit-style
>@@ -43,6 +43,7 @@ same line, but it is far from perfect (in either direction).
> """
> 
> import codecs
>+import logging
> import os
> import os.path
> import sys
>@@ -50,13 +51,28 @@ import sys
> import webkitpy.style.checker as checker
> from webkitpy.style_references import SimpleScm
> 
>+_log = logging.getLogger("check-webkit-style")
>+
> def main():
>     # Change stderr to write with replacement characters so we don't die
>     # if we try to print something containing non-ASCII characters.
>-    sys.stderr = codecs.StreamReaderWriter(sys.stderr,
>-                                           codecs.getreader('utf8'),
>-                                           codecs.getwriter('utf8'),
>-                                           'replace')
>+    stderr = codecs.StreamReaderWriter(sys.stderr,
>+                                       codecs.getreader('utf8'),
>+                                       codecs.getwriter('utf8'),
>+                                       'replace')
>+    # Setting an "encoding" attribute on the stream is necessary to
>+    # prevent the logging module from raising an error.  See
>+    # the checker.configure_logging() function for more information.
>+    stderr.encoding = "UTF-8"
>+
>+    # FIXME: Change webkitpy.style so that we do not need to overwrite
>+    #        the global sys.stderr.  This involves updating the code to
>+    #        accept a stream parameter where necessary, and not calling
>+    #        sys.stderr explicitly anywhere.
>+    sys.stderr = stderr
>+
>+    checker.configure_logging(stderr)
>+
>     parser = checker.check_webkit_style_parser()
>     (files, options) = parser.parse(sys.argv[1:])
> 
>@@ -78,7 +94,8 @@ def main():
>                 # FIXME: If the range is a "...", the code should find the common ancestor and
>                 # start there (see git diff --help for information about how ... usually works).
>                 commit = commit[:commit.find('..')]
>-                print >> sys.stderr, "Warning: Ranges are not supported for --git-commit. Checking all changes since %s.\n" % commit
>+                _log.warn("Ranges are not supported for --git-commit. "
>+                          "Checking all changes since %s.\n" % commit)
>             patch = scm.create_patch_since_local_commit(commit)
>         else:
>             patch = scm.create_patch()
>@@ -86,8 +103,8 @@ def main():
> 
>     error_count = style_checker.error_count
>     file_count = style_checker.file_count
>-    sys.stderr.write('Total errors found: %d in %d files\n'
>-                     % (error_count, file_count))
>+    _log.info("Total errors found: %d in %d files\n"
>+              % (error_count, file_count))
>     # We fail when style errors are found or there are no checked files.
>     sys.exit(error_count > 0 or file_count == 0)
> 
>diff --git a/WebKitTools/Scripts/webkitpy/style/checker.py b/WebKitTools/Scripts/webkitpy/style/checker.py
>index fe65f74..d80a58e 100644
>--- a/WebKitTools/Scripts/webkitpy/style/checker.py
>+++ b/WebKitTools/Scripts/webkitpy/style/checker.py
>@@ -30,6 +30,7 @@
> """Front end of some style-checker modules."""
> 
> import codecs
>+import logging
> import os.path
> import sys
> 
>@@ -45,6 +46,7 @@ from processors.cpp import CppProcessor
> from processors.python import PythonProcessor
> from processors.text import TextProcessor
> 
>+_log = logging.getLogger("webkitpy.style.checker")
> 
> # These are default option values for the command-line option parser.
> _DEFAULT_VERBOSITY = 1
>@@ -204,6 +206,85 @@ def check_webkit_style_configuration(options):
>                verbosity=options.verbosity)
> 
> 
>+# FIXME: Add support for more verbose logging for debug purposes.
>+#        This can use a formatter like the following, for example--
>+#
>+#        formatter = logging.Formatter("%(name)s: [%(levelname)s] %(message)s")
>+def configure_logging(stream):
>+    """Configure logging, and return the list of handlers added.
>+
>+    Configures the root logger to log INFO messages and higher.
>+    Configures WARNING messages and above to include the logging level
>+    and messages below WARNING not to include it.
>+
>+    This method returns a list of references to the logging handlers
>+    added so that the caller can later remove the handlers using
>+    logger.removeHandler.  This is primarily useful in testing
>+    situations where the caller may want to enable logging temporarily
>+    and then undo the enabling.
>+
>+    Args:
>+      stream: A file-like object to which to log.  The stream must
>+              define an "encoding" data attribute, or else logging
>+              raises an error.
>+
>+    """
>+    # If the stream does not define an "encoding" data attribute, the
>+    # logging module can throw an error like the following:
>+    #
>+    # Traceback (most recent call last):
>+    #   File "/System/Library/Frameworks/Python.framework/Versions/2.6/...
>+    #         lib/python2.6/logging/__init__.py", line 761, in emit
>+    #     self.stream.write(fs % msg.encode(self.stream.encoding))
>+    # LookupError: unknown encoding: unknown
>+
>+    # Handles logging.WARNING and above.
>+    error_handler = logging.StreamHandler(stream)
>+    error_handler.setLevel(logging.WARNING)
>+    formatter = logging.Formatter("%(levelname)s: %(message)s")
>+    error_handler.setFormatter(formatter)
>+
>+    # Handles records strictly below logging.WARNING.
>+    non_error_handler = logging.StreamHandler(stream)
>+    non_error_filter = _LevelLoggingFilter(logging.WARNING)
>+    non_error_handler.addFilter(non_error_filter)
>+    formatter = logging.Formatter("%(message)s")
>+    non_error_handler.setFormatter(formatter)
>+
>+    logger = logging.getLogger()
>+    logger.setLevel(logging.INFO)
>+
>+    handlers = [error_handler, non_error_handler]
>+
>+    for handler in handlers:
>+        logger.addHandler(handler)
>+
>+    return handlers
>+
>+
>+# FIXME: After we get more familiar with using more advanced logging
>+#        techniques like the filter class below, we may want to move
>+#        classes like this to a webkitpy.infrastructure.logging module.
>+class _LevelLoggingFilter(object):
>+
>+    """A logging filter for blocking records at or above a certain level."""
>+
>+    def __init__(self, logging_level):
>+        """Create a _LevelLoggingFilter.
>+
>+        Args:
>+          logging_level: The logging level cut-off.  Logging levels at
>+                         or above this level will not be logged.
>+
>+        """
>+        self._logging_level = logging_level
>+
>+    # The logging module requires that this method be defined.
>+    def filter(self, log_record):
>+        """Return whether given the LogRecord should be logged."""
>+        return log_record.levelno < self._logging_level
>+
>+
> # Enum-like idiom
> class FileType:
> 
>@@ -306,6 +387,8 @@ class ProcessorDispatcher(object):
>         return processor
> 
> 
>+# FIXME: Remove the stderr_write attribute from this class and replace
>+#        its use with calls to a logging module logger.
> class StyleCheckerConfiguration(object):
> 
>     """Stores configuration values for the StyleChecker class.
>@@ -423,9 +506,6 @@ class StyleChecker(object):
>         self.error_count = 0
>         self.file_count = 0
> 
>-    def _stderr_write(self, message):
>-        self._configuration.stderr_write(message)
>-
>     def _increment_error_count(self):
>         """Increment the total count of reported errors."""
>         self.error_count += 1
>@@ -453,11 +533,15 @@ class StyleChecker(object):
>             contents = file.read()
> 
>         except IOError:
>-            self._stderr_write("Skipping input '%s': Can't open for reading\n" % file_path)
>+            message = 'Could not read file. Skipping: "%s"' % file_path
>+            _log.warn(message)
>             return
> 
>         lines = contents.split("\n")
> 
>+        # FIXME: Make a CarriageReturnProcessor for this logic, and put
>+        #        it in processors.common.  The process() method should
>+        #        return the lines with the carriage returns stripped.
>         for line_number in range(len(lines)):
>             # FIXME: We should probably use the SVN "eol-style" property
>             #        or a white list to decide whether or not to do
>@@ -504,8 +588,8 @@ class StyleChecker(object):
>         if dispatcher.should_skip_without_warning(file_path):
>             return
>         if dispatcher.should_skip_with_warning(file_path):
>-            self._stderr_write('Ignoring "%s": this file is exempt from the '
>-                               "style guide.\n" % file_path)
>+            _log.warn('File exempt from style guide. Skipping: "%s"'
>+                      % file_path)
>             return
> 
>         verbosity = self._configuration.verbosity
>diff --git a/WebKitTools/Scripts/webkitpy/style/checker_unittest.py b/WebKitTools/Scripts/webkitpy/style/checker_unittest.py
>index ba82670..2fb27c4 100755
>--- a/WebKitTools/Scripts/webkitpy/style/checker_unittest.py
>+++ b/WebKitTools/Scripts/webkitpy/style/checker_unittest.py
>@@ -34,6 +34,7 @@
> 
> """Unit tests for style.py."""
> 
>+import logging
> import unittest
> 
> import checker as style
>@@ -43,6 +44,7 @@ from checker import _PATH_RULES_SPECIFIER as PATH_RULES_SPECIFIER
> from checker import _all_categories
> from checker import check_webkit_style_configuration
> from checker import check_webkit_style_parser
>+from checker import configure_logging
> from checker import ProcessorDispatcher
> from checker import StyleChecker
> from checker import StyleCheckerConfiguration
>@@ -55,6 +57,70 @@ from processors.python import PythonProcessor
> from processors.text import TextProcessor
> 
> 
>+class CheckerTestBase(unittest.TestCase):
>+
>+    """Supports the unit testing of log messages."""
>+
>+    class _UnitTestLoggingStream(object):
>+
>+        """Return a file-like object for unit testing logging."""
>+
>+        def __init__(self):
>+            self.messages = []
>+            """Each element represents a log message passed to the stream."""
>+
>+        def write(self, message):
>+            self.messages.append(message)
>+
>+        def flush(self):
>+            pass
>+
>+    def _log(self):
>+        return logging.getLogger()
>+
>+    def setUp(self):
>+        stream = self._UnitTestLoggingStream()
>+        self._log_stream = stream
>+        self._handlers = configure_logging(stream)
>+
>+    def tearDown(self):
>+        """Reset logging to its original state.
>+
>+        This method ensures that the logging configuration set up
>+        for a unit test does not effect logging in other unit tests.
>+
>+        """
>+        log = self._log()
>+        for handler in self._handlers:
>+            log.removeHandler(handler)
>+
>+    def assert_log_messages(self, messages):
>+        """Assert that the logged messages equal the given messages."""
>+        self.assertEquals(messages, self._log_stream.messages)
>+
>+
>+class ConfigureLoggingTest(CheckerTestBase):
>+
>+    """Tests the configure_logging() function."""
>+
>+    def test_warning_message(self):
>+        self._log().warn("test message")
>+        self.assert_log_messages(["WARNING: test message\n"])
>+
>+    def test_info_message(self):
>+        self._log().info("test message")
>+        self.assert_log_messages(["test message\n"])
>+
>+    def test_debug_message(self):
>+        self._log().debug("test message")
>+        self.assert_log_messages([])
>+
>+    def test_two_messages(self):
>+        self._log().info("message1")
>+        self._log().info("message2")
>+        self.assert_log_messages(["message1\n", "message2\n"])
>+
>+
> class GlobalVariablesTest(unittest.TestCase):
> 
>     """Tests validity of the global variables."""
>@@ -435,7 +501,7 @@ class StyleCheckerTest(unittest.TestCase):
>         self.assertEquals(style_checker.file_count, 0)
> 
> 
>-class StyleCheckerCheckFileTest(unittest.TestCase):
>+class StyleCheckerCheckFileTest(CheckerTestBase):
> 
>     """Test the check_file() method of the StyleChecker class.
> 
>@@ -461,6 +527,7 @@ class StyleCheckerCheckFileTest(unittest.TestCase):
> 
>     """
>     def setUp(self):
>+        CheckerTestBase.setUp(self)
>         self.got_file_path = None
>         self.got_handle_style_error = None
>         self.got_processor = None
>@@ -538,8 +605,9 @@ class StyleCheckerCheckFileTest(unittest.TestCase):
> 
>         # Check the outcome.
>         self.call_check_file(file_path)
>-        self.assert_attributes(None, None, None,
>-                               'Ignoring "gtk2drawing.c": this file is exempt from the style guide.\n')
>+        self.assert_attributes(None, None, None, "")
>+        self.assert_log_messages(["WARNING: File exempt from style guide. "
>+                                  'Skipping: "gtk2drawing.c"\n'])
> 
>     def test_check_file_on_non_skipped(self):
>

Actions: View | Formatted Diff | Diff

Attachments on bug 35484: 49687 | 49688 | 49691 | 49787 | 49788 | 49805