Bug 34676 – check-webkit-style: Switch from using getopt.getopt() to optparse.OptionsParser

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chris Jerdonek]

This was originally suggested here:

https://bugs.webkit.org/show_bug.cgi?id=33045#c17

[Chris Jerdonek]

Created attachment 52270 [details]
Proposed patch

Note that check-webkit-style is now doing the pep8 check for Python files.

[Chris Jerdonek]

To assist the reviewing process, this is what help looks like with this patch:

> check-webkit-style --min-confidence=xxx
Usage: check-webkit-style [--help] [options] [path1] [path2] ...

Overview:
  Check coding style according to WebKit style guidelines:

      http://webkit.org/coding/coding-style.html

  Path arguments can be files and directories.  If neither a git commit nor
  paths are passed, then all changes in your source control working directory
  are checked.

Style errors:
  This script assigns to every style error a confidence score from 1-5 and
  a category name.  A confidence score of 5 means the error is certainly
  a problem, and 1 means it could be fine.

  Category names appear in error messages in brackets, for example
  [whitespace/indent].  See the options section below for an option that
  displays all available categories and which are reported by default.

Filters:
  Use filters to configure what errors to report.  Filters are specified using
  a comma-separated list of boolean filter rules.  The script reports errors
  in a category if the category passes the filter, as described below.

  All categories start out passing.  Boolean filter rules are then evaluated
  from left to right, with later rules taking precedence.  For example, the
  rule "+foo" passes any category that starts with "foo", and "-foo" fails
  any such category.  The filter input "-whitespace,+whitespace/braces" fails
  the category "whitespace/tab" and passes "whitespace/braces".

  Examples: --filter=-whitespace,+whitespace/braces
            --filter=-whitespace,-runtime/printf,+runtime/printf_format
            --filter=-,+build/include_what_you_use

Paths:
  Certain style-checking behavior depends on the paths relative to
  the WebKit source root of the files being checked.  For example,
  certain types of errors may be handled differently for files in
  WebKit/gtk/webkit/ (e.g. by suppressing "readability/naming" errors
  for files in this directory).

  Consequently, if the path relative to the source root cannot be
  determined for a file being checked, then style checking may not
  work correctly for that file.  This can occur, for example, if no
  WebKit checkout can be found, or if the source root can be detected,
  but one of the files being checked lies outside the source tree.

  If a WebKit checkout can be detected and all files being checked
  are in the source tree, then all paths will automatically be
  converted to paths relative to the source root prior to checking.
  This is also useful for display purposes.

  Currently, this command can detect the source root only if the
  command is run from within a WebKit checkout (i.e. if the current
  working directory is below the root of a checkout).  In particular,
  it is not recommended to run this script from a directory outside
  a checkout.

  Running this script from a top-level WebKit source directory and
  checking only files in the source tree will ensure that all style
  checking behaves correctly -- whether or not a checkout can be
  detected.  This is because all file paths will already be relative
  to the source root and so will not need to be converted.

Options:
  -h, --help            show this help message and exit
  -f RULES, --filter-rules=RULES
                        set a filter to control what categories of style
                        errors to report.  Specify a filter using a comma-
                        delimited list of boolean filter rules, for example "
                        --filter -whitespace,+whitespace/braces".  Pass a
                        question mark ("?") to display all categories and
                        which are enabled by default.
  -g COMMIT, --git-commit=COMMIT, --git-diff=COMMIT, --git-since=COMMIT
                        check all changes after the given git commit.
  -m INT, --min-confidence=INT
                        set the minimum confidence of style errors to report.
                        Can be an integer 1-5, with 1 displaying all errors.
                        Defaults to 1.
  -o FORMAT, --output-format=FORMAT
                        set the output format, which can be "emacs" or "vs7"
                        (for Visual Studio).  Defaults to "emacs".
  -v, --verbose         enable verbose logging.

This script can miss errors and does not substitute for code review.
ERROR: option --min-confidence: invalid integer value: 'xxx'
>

[Shinichiro Hamaji]

Comment on attachment 52270 [details]
Proposed patch

This change looks good, I couldn't look the patch closely yet, though.

I'm not sure if --filter=? is better than --filter= . How about outputting the list of available filters for all invalid filters such as "", "?", "runtime/arrays", and etc?

Also, as this patch changes the behavior of --filter, I think it's nice to mention this change in your ChangeLog.

> +        sys.exit(2)

Why exit(2) is better than 1?

> This script can miss errors and does not substitute for code review.
> ERROR: option --min-confidence: invalid integer value: 'xxx'

Cannot we have one blank line before the "ERROR: ..." line?

Putting r- for now to reduce the number of patches in our review queue, but I'll re-put r+ if I'm convinced.

[Chris Jerdonek]

(In reply to comment #3)
> I'm not sure if --filter=? is better than --filter= . How about outputting the
> list of available filters for all invalid filters such as "", "?",
> "runtime/arrays", and etc?

I couldn't seem to get --filter= to work before.  I thought this was because OptionParser has different parsing semantics than getopt.  However, it seems to be working fine now (I must have been doing something wrong before), so I can change it back to --filter.  Thanks for asking about this.

I think we should maybe keep the existing behavior of showing an error message in the case of a bad filter?

> check-webkit-style --filter=a 
Usage: check-webkit-style [--help] [options] [path1] [path2] ...

Overview:
  Check coding style according to WebKit style guidelines:
...
This script can miss errors and does not substitute for code review.
ERROR: Invalid filter rule "a": every rule must start with + or -.

Maybe there is a way to distinguish between bad filter syntax (e.g. the above) vs. unrecognized category names.  For the latter it would make sense to provide the list.  I'll look into how easy it would be to do this.

> Also, as this patch changes the behavior of --filter, I think it's nice to
> mention this change in your ChangeLog.
> 
> > +        sys.exit(2)
> 
> Why exit(2) is better than 1?

This was to match Python's optparse.OptionParser existing behavior, since we are replacing/overriding OptionParser.error():

(from optparse.py)

    def exit(self, status=0, msg=None):
        if msg:
            sys.stderr.write(msg)
        sys.exit(status)

    def error(self, msg):
        """error(msg : string)

        Print a usage message incorporating 'msg' to stderr and exit.
        If you override this in a subclass, it should not return -- it
        should either exit or raise an exception.
        """
        self.print_usage(sys.stderr)
        self.exit(2, "%s: error: %s\n" % (self.get_prog_name(), msg))

> 
> > This script can miss errors and does not substitute for code review.
> > ERROR: option --min-confidence: invalid integer value: 'xxx'
> 
> Cannot we have one blank line before the "ERROR: ..." line?

Yes, we can certainly do that.  I noticed before that the OptionParser class strips all white space from the epilog string if you try to include it there, which is why I left it that way.  But since we're already concatenating the help output ourselves, we can now easily circumvent that issue.

Thanks for the quick feedback!

> 
> Putting r- for now to reduce the number of patches in our review queue, but
> I'll re-put r+ if I'm convinced.

[Chris Jerdonek]

(In reply to comment #4)
> > > +        sys.exit(2)
> > 
> > Why exit(2) is better than 1?

FYI, the Python documentation says this--

"Some systems have a convention for assigning specific meanings to specific exit codes, but these are generally underdeveloped; Unix programs generally use 2 for command line syntax errors and 1 for all other kind of errors. If another type of object is passed, None is equivalent to passing zero, and any other object is printed to sys.stderr and results in an exit code of 1. In particular, sys.exit("some error message") is a quick way to exit a program when an error occurs."

(from http://docs.python.org/library/sys.html#sys.exit )

[Shinichiro Hamaji]

> I think we should maybe keep the existing behavior of showing an error message
> in the case of a bad filter?

Yeah, I think it's fine.

> > Also, as this patch changes the behavior of --filter, I think it's nice to
> > mention this change in your ChangeLog.
> > 
> > > +        sys.exit(2)
> > 
> > Why exit(2) is better than 1?
> 
> This was to match Python's optparse.OptionParser existing behavior, since we
> are replacing/overriding OptionParser.error():

Thanks for the description! I think this info is useful if it is written in the code. So, could you add a brief comment to describe why our exit code should be 2?

[Chris Jerdonek]

(In reply to comment #4)
> (In reply to comment #3)
> > I'm not sure if --filter=? is better than --filter= . How about outputting the
> > list of available filters for all invalid filters such as "", "?",
> > "runtime/arrays", and etc?
> 
> I couldn't seem to get --filter= to work before.  I thought this was because
> OptionParser has different parsing semantics than getopt.  However, it seems to
> be working fine now (I must have been doing something wrong before), so I can
> change it back to --filter.  Thanks for asking about this.

Maybe what it was before is that I didn't like how "-f=" doesn't work while "--filter=" does.  I'll make a note in the help string that you need to do "-f ''" if using the short form.

[Chris Jerdonek]

Created attachment 52378 [details]
Proposed patch 2

[Shinichiro Hamaji]

Comment on attachment 52378 [details]
Proposed patch 2

Looks good. Thanks for considering my comments!

[Chris Jerdonek]

Comment on attachment 52378 [details]
Proposed patch 2

Clearing flags on attachment: 52378

Committed r56987: <http://trac.webkit.org/changeset/56987>

[Chris Jerdonek]

All reviewed patches have been landed.  Closing bug.