Changeset 56987 in webkit


Ignore:
Timestamp:
Apr 2, 2010 1:27:22 AM (14 years ago)
Author:
Chris Jerdonek
Message:

2010-04-02 Chris Jerdonek <Chris Jerdonek>

Reviewed by Shinichiro Hamaji.

Re-wrote check-webkit-style's argument parsing code to use
Python's optparser module and more uniform error-handling logic.

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

  • Scripts/webkitpy/style/optparser.py:
    • Removed "option help" from check-webkit-style's usage string since that is provided separately by the OptionParser class.
    • Also changed the usage string from a function to a constant string _USAGE.
    • Added an _EPILOG string which renders after OptionParser's usage string and option help.
    • In the ArgumentParser class:
      • Changed the constructor's stderr_write parameter to a mock_stderr since the OptionParser accepts a sys.stderr substitute rather than a sys.stderr.write substitute.
      • Changed the constructor to set a _parser data attribute with an OptionParser instance.
      • Added a _create_option_parser() method which instantiates the OptionParser.
      • Updated _exit_with_help() to interact with the OptionParser's help method.
      • Updated the parse() method as necessary. Also changed the raising of ValueErrors to calls to _exit_with_help().
  • Scripts/webkitpy/style/optparser_unittest.py:
    • Removed the CreateUsageTest class since the create_usage method was replaced by a constant string.
    • Added a _MockStdErr class to the ArgumentParserTest class.
    • Updated the unit tests as necessary.
Location:
trunk/WebKitTools
Files:
3 edited

Legend:

Unmodified
Added
Removed
  • trunk/WebKitTools/ChangeLog

    r56986 r56987  
     12010-04-02  Chris Jerdonek  <cjerdonek@webkit.org>
     2
     3        Reviewed by Shinichiro Hamaji.
     4
     5        Re-wrote check-webkit-style's argument parsing code to use
     6        Python's optparser module and more uniform error-handling logic.
     7
     8        https://bugs.webkit.org/show_bug.cgi?id=34676
     9
     10        * Scripts/webkitpy/style/optparser.py:
     11          - Removed "option help" from check-webkit-style's usage string
     12            since that is provided separately by the OptionParser class.
     13          - Also changed the usage string from a function to a constant
     14            string _USAGE.
     15          - Added an _EPILOG string which renders after OptionParser's
     16            usage string and option help.
     17          - In the ArgumentParser class:
     18            - Changed the constructor's stderr_write parameter to a
     19              mock_stderr since the OptionParser accepts a sys.stderr
     20              substitute rather than a sys.stderr.write substitute.
     21            - Changed the constructor to set a _parser data attribute with
     22              an OptionParser instance.
     23            - Added a _create_option_parser() method which instantiates
     24              the OptionParser.
     25            - Updated _exit_with_help() to interact with the OptionParser's
     26              help method.
     27            - Updated the parse() method as necessary.  Also changed the
     28              raising of ValueErrors to calls to _exit_with_help().
     29
     30        * Scripts/webkitpy/style/optparser_unittest.py:
     31          - Removed the CreateUsageTest class since the create_usage method
     32            was replaced by a constant string.
     33          - Added a _MockStdErr class to the ArgumentParserTest class.
     34          - Updated the unit tests as necessary.
     35
     36
    1372010-04-02  Adam Barth  <abarth@webkit.org>
    238
  • trunk/WebKitTools/Scripts/webkitpy/style/optparser.py

    r56692 r56987  
    2323"""Supports the parsing of command-line options for check-webkit-style."""
    2424
    25 import getopt
     25from optparse import OptionParser
    2626import logging
    2727import os.path
     
    3333_log = logging.getLogger(__name__)
    3434
    35 
    36 def _create_usage(default_options):
    37     """Return the usage string to display for command help.
    38 
    39     Args:
    40       default_options: A DefaultCommandOptionValues instance.
    41 
    42     """
    43     usage = """
    44 Syntax: %(program_name)s [--filter=-x,+y,...] [--git-commit=<SingleCommit>]
    45         [--min-confidence=#] [--output=vs7] [--verbose] [file or directory] ...
    46 
    47   The style guidelines this tries to follow are here:
    48     http://webkit.org/coding/coding-style.html
    49 
    50   Every style error is given a confidence score from 1-5, with 5 meaning
    51   we are certain of the problem, and 1 meaning it could be a legitimate
    52   construct.  This can miss some errors and does not substitute for
    53   code review.
    54 
    55   To prevent specific lines from being linted, add a '// NOLINT' comment to the
    56   end of the line.
    57 
    58   Linted extensions are .cpp, .c and .h.  Other file types are ignored.
    59 
    60   The file parameter is optional and accepts multiple files.  Leaving
    61   out the file parameter applies the check to all files considered changed
    62   by your source control management system.
    63 
    64   Flags:
    65 
    66     filter=-x,+y,...
    67       A comma-separated list of boolean filter rules used to filter
    68       which categories of style guidelines to check.  The script checks
    69       a category if the category passes the filter rules, as follows.
    70 
    71       Any webkit category starts out passing.  All filter rules are then
    72       evaluated left to right, with later rules taking precedence.  For
    73       example, the rule "+foo" passes any category that starts with "foo",
    74       and "-foo" fails any such category.  The filter input "-whitespace,
    75       +whitespace/braces" fails the category "whitespace/tab" and passes
    76       "whitespace/braces".
    77 
    78       Examples: --filter=-whitespace,+whitespace/braces
    79                 --filter=-whitespace,-runtime/printf,+runtime/printf_format
    80                 --filter=-,+build/include_what_you_use
    81 
    82       Category names appear in error messages in brackets, for example
    83       [whitespace/indent].  To see a list of all categories available to
    84       %(program_name)s, along with which are enabled by default, pass
    85       the empty filter as follows:
    86          --filter=
    87 
    88     git-commit=<SingleCommit>
    89       Checks the style of everything from the given commit to the local tree.
    90 
    91     min-confidence=#
    92       An integer between 1 and 5 inclusive that is the minimum confidence
    93       level of style errors to report.  In particular, the value 1 displays
    94       all errors.  The default is %(default_min_confidence)s.
    95 
    96     output=vs7
    97       The output format, which may be one of
    98         emacs : to ease emacs parsing
    99         vs7   : compatible with Visual Studio
    100       Defaults to "%(default_output_format)s". Other formats are unsupported.
    101 
    102     verbose
    103       Logging is verbose if this flag is present.
    104 
    105 Path considerations:
    106 
     35_USAGE = """usage: %prog [--help] [options] [path1] [path2] ...
     36
     37Overview:
     38  Check coding style according to WebKit style guidelines:
     39
     40      http://webkit.org/coding/coding-style.html
     41
     42  Path arguments can be files and directories.  If neither a git commit nor
     43  paths are passed, then all changes in your source control working directory
     44  are checked.
     45
     46Style errors:
     47  This script assigns to every style error a confidence score from 1-5 and
     48  a category name.  A confidence score of 5 means the error is certainly
     49  a problem, and 1 means it could be fine.
     50
     51  Category names appear in error messages in brackets, for example
     52  [whitespace/indent].  See the options section below for an option that
     53  displays all available categories and which are reported by default.
     54
     55Filters:
     56  Use filters to configure what errors to report.  Filters are specified using
     57  a comma-separated list of boolean filter rules.  The script reports errors
     58  in a category if the category passes the filter, as described below.
     59
     60  All categories start out passing.  Boolean filter rules are then evaluated
     61  from left to right, with later rules taking precedence.  For example, the
     62  rule "+foo" passes any category that starts with "foo", and "-foo" fails
     63  any such category.  The filter input "-whitespace,+whitespace/braces" fails
     64  the category "whitespace/tab" and passes "whitespace/braces".
     65
     66  Examples: --filter=-whitespace,+whitespace/braces
     67            --filter=-whitespace,-runtime/printf,+runtime/printf_format
     68            --filter=-,+build/include_what_you_use
     69
     70Paths:
    10771  Certain style-checking behavior depends on the paths relative to
    10872  the WebKit source root of the files being checked.  For example,
     
    13296  checking behaves correctly -- whether or not a checkout can be
    13397  detected.  This is because all file paths will already be relative
    134   to the source root and so will not need to be converted.
    135 
    136 """ % {'program_name': os.path.basename(sys.argv[0]),
    137        'default_min_confidence': default_options.min_confidence,
    138        'default_output_format': default_options.output_format}
    139 
    140     return usage
     98  to the source root and so will not need to be converted."""
     99
     100_EPILOG = ("This script can miss errors and does not substitute for "
     101           "code review.")
    141102
    142103
     
    263224
    264225
    265 # FIXME: Replace the use of getopt.getopt() with optparse.OptionParser.
    266226class ArgumentParser(object):
    267227
     
    288248                 default_options,
    289249                 base_filter_rules=None,
    290                  create_usage=None,
    291                  stderr_write=None):
     250                 mock_stderr=None,
     251                 usage=None):
    292252        """Create an ArgumentParser instance.
    293253
     
    311271        if base_filter_rules is None:
    312272            base_filter_rules = []
    313         if create_usage is None:
    314             create_usage = _create_usage
    315         if stderr_write is None:
    316             stderr_write = sys.stderr.write
     273        stderr = sys.stderr if mock_stderr is None else mock_stderr
     274        if usage is None:
     275            usage = _USAGE
    317276
    318277        self._all_categories = all_categories
    319278        self._base_filter_rules = base_filter_rules
     279
    320280        # FIXME: Rename these to reflect that they are internal.
    321         self.create_usage = create_usage
    322281        self.default_options = default_options
    323         self.stderr_write = stderr_write
    324 
    325     def _exit_with_usage(self, error_message=''):
     282        self.stderr_write = stderr.write
     283
     284        self._parser = self._create_option_parser(stderr=stderr,
     285            usage=usage,
     286            default_min_confidence=self.default_options.min_confidence,
     287            default_output_format=self.default_options.output_format)
     288
     289    def _create_option_parser(self, stderr, usage,
     290                              default_min_confidence, default_output_format):
     291        # Since the epilog string is short, it is not necessary to replace
     292        # the epilog string with a mock epilog string when testing.
     293        # For this reason, we use _EPILOG directly rather than passing it
     294        # as an argument like we do for the usage string.
     295        parser = OptionParser(usage=usage, epilog=_EPILOG)
     296
     297        filter_help = ('set a filter to control what categories of style '
     298                       'errors to report.  Specify a filter using a comma-'
     299                       'delimited list of boolean filter rules, for example '
     300                       '"--filter -whitespace,+whitespace/braces".  To display '
     301                       'all categories and which are enabled by default, pass '
     302                       """no value (e.g. '-f ""' or '--filter=').""")
     303        parser.add_option("-f", "--filter-rules", metavar="RULES",
     304                          dest="filter_value", help=filter_help)
     305
     306        git_help = "check all changes after the given git commit."
     307        parser.add_option("-g", "--git-commit", "--git-diff", "--git-since",
     308                          metavar="COMMIT", dest="git_since", help=git_help,)
     309
     310        min_confidence_help = ("set the minimum confidence of style errors "
     311                               "to report.  Can be an integer 1-5, with 1 "
     312                               "displaying all errors.  Defaults to %default.")
     313        parser.add_option("-m", "--min-confidence", metavar="INT",
     314                          type="int", dest="min_confidence",
     315                          default=default_min_confidence,
     316                          help=min_confidence_help)
     317
     318        output_format_help = ('set the output format, which can be "emacs" '
     319                              'or "vs7" (for Visual Studio).  '
     320                              'Defaults to "%default".')
     321        parser.add_option("-o", "--output-format", metavar="FORMAT",
     322                          choices=["emacs", "vs7"],
     323                          dest="output_format", default=default_output_format,
     324                          help=output_format_help)
     325
     326        verbose_help = "enable verbose logging."
     327        parser.add_option("-v", "--verbose", dest="is_verbose", default=False,
     328                          action="store_true", help=verbose_help)
     329
     330        # Override OptionParser's error() method so that option help will
     331        # also display when an error occurs.  Normally, just the usage
     332        # string displays and not option help.
     333        parser.error = self._exit_with_help
     334
     335        # Override OptionParser's print_help() method so that help output
     336        # does not render to the screen while running unit tests.
     337        print_help = parser.print_help
     338        parser.print_help = lambda: print_help(file=stderr)
     339
     340        return parser
     341
     342    def _exit_with_help(self, error_message=None):
    326343        """Exit and print a usage string with an optional error message.
    327344
     
    330347
    331348        """
    332         usage = self.create_usage(self.default_options)
    333         self.stderr_write(usage)
     349        # The method format_help() includes both the usage string and
     350        # the flag options.
     351        help = self._parser.format_help()
     352        # Separate help from the error message with a single blank line.
     353        self.stderr_write(help + "\n")
    334354        if error_message:
    335             sys.exit('\nFATAL ERROR: ' + error_message)
    336         else:
    337             sys.exit(1)
     355            _log.error(error_message)
     356
     357        # Since we are using this method to replace/override the Python
     358        # module optparse's OptionParser.error() method, we match its
     359        # behavior and exit with status code 2.
     360        #
     361        # As additional background, Python documentation says--
     362        #
     363        # "Unix programs generally use 2 for command line syntax errors
     364        #  and 1 for all other kind of errors."
     365        #
     366        # (from http://docs.python.org/library/sys.html#sys.exit )
     367        sys.exit(2)
    338368
    339369    def _exit_with_categories(self):
     
    382412
    383413        """
    384         is_verbose = False
    385         min_confidence = self.default_options.min_confidence
    386         output_format = self.default_options.output_format
    387 
    388         # The flags that the CommandOptionValues class supports.
    389         flags = ['filter=', 'git-commit=', 'help', 'min-confidence=',
    390                  'output=', 'verbose']
    391 
    392         try:
    393             (opts, paths) = getopt.getopt(args, '', flags)
    394         except getopt.GetoptError, err:
    395             # FIXME: Settle on an error handling approach: come up
    396             #        with a consistent guideline as to when and whether
    397             #        a ValueError should be raised versus calling
    398             #        sys.exit when needing to interrupt execution.
    399             self._exit_with_usage('Invalid arguments: %s' % err)
    400 
    401         git_commit = None
    402         filter_rules = []
    403 
    404         for (opt, val) in opts:
    405             # Process --help first (out of alphabetical order).
    406             if opt == '--help':
    407                 self._exit_with_usage()
    408             elif opt == '--filter':
    409                 if not val:
    410                     self._exit_with_categories()
    411                 filter_rules = self._parse_filter_flag(val)
    412             elif opt == '--git-commit':
    413                 git_commit = val
    414             elif opt == '--min-confidence':
    415                 min_confidence = val
    416             elif opt == '--output':
    417                 output_format = val
    418             elif opt == "--verbose":
    419                 is_verbose = True
    420             else:
    421                 # We should never get here because getopt.getopt()
    422                 # raises an error in this case.
    423                 raise ValueError('Invalid option: "%s"' % opt)
    424 
    425         # Check validity of resulting values.
     414        (options, paths) = self._parser.parse_args(args=args)
     415
     416        filter_value = options.filter_value
     417        git_commit = options.git_since
     418        is_verbose = options.is_verbose
     419        min_confidence = options.min_confidence
     420        output_format = options.output_format
     421
     422        if filter_value is not None and not filter_value:
     423            # Then the user explicitly passed no filter, for
     424            # example "-f ''" or "--filter=".
     425            self._exit_with_categories()
     426
     427        # Validate user-provided values.
     428
     429        # FIXME: Move the checkout error outside of this module.  The parse
     430        #        method should not need to know whether a checkout was found.
    426431        if not found_checkout and not paths:
    427432            _log.error("WebKit checkout not found: You must run this script "
     
    430435            sys.exit(1)
    431436
    432         if paths and (git_commit != None):
    433             self._exit_with_usage('It is not possible to check files and a '
    434                                   'specific commit at the same time.')
    435 
    436         if output_format not in ('emacs', 'vs7'):
    437             raise ValueError('Invalid --output value "%s": The only '
    438                              'allowed output formats are emacs and vs7.' %
    439                              output_format)
    440 
    441         validate_filter_rules(filter_rules, self._all_categories)
     437        if paths and git_commit:
     438            self._exit_with_help('You cannot provide both paths and a git '
     439                                 'commit at the same time.')
    442440
    443441        min_confidence = int(min_confidence)
    444442        if (min_confidence < 1) or (min_confidence > 5):
    445             raise ValueError('Invalid --min-confidence value %s: value must '
    446                              'be between 1 and 5 inclusive.' % min_confidence)
     443            self._exit_with_help('option --min-confidence: invalid integer: '
     444                                 '%s: value must be between 1 and 5'
     445                                 % min_confidence)
     446
     447        if filter_value:
     448            filter_rules = self._parse_filter_flag(filter_value)
     449        else:
     450            filter_rules = []
     451
     452        try:
     453            validate_filter_rules(filter_rules, self._all_categories)
     454        except ValueError, err:
     455            self._exit_with_help(err)
    447456
    448457        options = CommandOptionValues(filter_rules=filter_rules,
  • trunk/WebKitTools/Scripts/webkitpy/style/optparser_unittest.py

    r56692 r56987  
    2525import unittest
    2626
    27 from optparser import _create_usage
    2827from optparser import ArgumentParser
    2928from optparser import ArgumentPrinter
     
    3130from optparser import DefaultCommandOptionValues
    3231from webkitpy.style_references import LogTesting
    33 
    34 
    35 class CreateUsageTest(unittest.TestCase):
    36 
    37     """Tests the _create_usage() function."""
    38 
    39     def test_create_usage(self):
    40         default_options = DefaultCommandOptionValues(min_confidence=3,
    41                                                      output_format="vs7")
    42         # Exercise the code path to make sure the function does not error out.
    43         _create_usage(default_options)
    4432
    4533
     
    7765    """Test the ArgumentParser class."""
    7866
     67    class _MockStdErr(object):
     68
     69        def write(self, message):
     70            # We do not want the usage string or style categories
     71            # to print during unit tests, so print nothing.
     72            return
     73
    7974    def setUp(self):
    8075        self._log = LogTesting.setUp(self)
     
    9792    def _create_parser(self):
    9893        """Return an ArgumentParser instance for testing."""
    99         def stderr_write(message):
    100             # We do not want the usage string or style categories
    101             # to print during unit tests, so print nothing.
    102             return
    103 
    10494        default_options = self._create_defaults()
    10595
    10696        all_categories = ["build" ,"whitespace"]
     97
     98        mock_stderr = self._MockStdErr()
     99
    107100        return ArgumentParser(all_categories=all_categories,
    108101                              base_filter_rules=[],
    109102                              default_options=default_options,
    110                               stderr_write=stderr_write)
     103                              mock_stderr=mock_stderr,
     104                              usage="test usage")
    111105
    112106    def test_parse_documentation(self):
     
    126120        # Pass an unsupported argument.
    127121        self.assertRaises(SystemExit, parse, ['--bad'])
    128 
    129         self.assertRaises(ValueError, parse, ['--min-confidence=bad'])
    130         self.assertRaises(ValueError, parse, ['--min-confidence=0'])
    131         self.assertRaises(ValueError, parse, ['--min-confidence=6'])
     122        self._log.assertMessages(['ERROR: no such option: --bad\n'])
     123
     124        self.assertRaises(SystemExit, parse, ['--min-confidence=bad'])
     125        self._log.assertMessages(["ERROR: option --min-confidence: invalid "
     126                                  "integer value: 'bad'\n"])
     127        self.assertRaises(SystemExit, parse, ['--min-confidence=0'])
     128        self._log.assertMessages(['ERROR: option --min-confidence: invalid '
     129                                  'integer: 0: value must be between 1 and '
     130                                  '5\n'])
     131        self.assertRaises(SystemExit, parse, ['--min-confidence=6'])
     132        self._log.assertMessages(['ERROR: option --min-confidence: invalid '
     133                                  'integer: 6: value must be between 1 and '
     134                                  '5\n'])
    132135        parse(['--min-confidence=1']) # works
    133136        parse(['--min-confidence=5']) # works
    134137
    135         self.assertRaises(ValueError, parse, ['--output=bad'])
     138        self.assertRaises(SystemExit, parse, ['--output=bad'])
     139        self._log.assertMessages(["ERROR: option --output-format: invalid "
     140                                  "choice: 'bad' (choose from 'emacs', "
     141                                  "'vs7')\n"])
    136142        parse(['--output=vs7']) # works
    137143
    138144        # Pass a filter rule not beginning with + or -.
    139         self.assertRaises(ValueError, parse, ['--filter=build'])
     145        self.assertRaises(SystemExit, parse, ['--filter=build'])
     146        self._log.assertMessages(['ERROR: Invalid filter rule "build": every '
     147                                  'rule must start with + or -.\n'])
    140148        parse(['--filter=+build']) # works
    141149        # Pass files and git-commit at the same time.
    142         self.assertRaises(SystemExit, parse, ['--git-commit=', 'file.txt'])
     150        self.assertRaises(SystemExit, parse, ['--git-commit=committish',
     151                                              'file.txt'])
     152        self._log.assertMessages(['ERROR: You cannot provide both paths and '
     153                                  'a git commit at the same time.\n'])
     154
     155    def test_parse_no_paths_without_scm(self):
     156        parse = self._parse
    143157
    144158        # Paths must be passed when found_scm is False.
     
    148162                    "passing specific paths to check.\n"]
    149163        self._log.assertMessages(messages)
    150         self.assertRaises(SystemExit, parse, ['--verbose=3'], found_scm=False)
     164        # Test one more variant.
     165        self.assertRaises(SystemExit, parse, ['--output=vs7'], found_scm=False)
     166        self._log.assertMessages(messages)
    151167
    152168    def test_parse_default_arguments(self):
Note: See TracChangeset for help on using the changeset viewer.