Bug 32966 – check-webkit-style: Refactor style.parse_arguments so that it has no global variable side effects.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chris Jerdonek]

Continuation of the following report (for Patch 2 of 2):

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

[Chris Jerdonek]

Created attachment 45541 [details]
Proposed patch

This is part 2 of the patch originally proposed in the bug report mentioned in the first comment.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45541 [details] without any errors.

[Shinichiro Hamaji]

Comment on attachment 45541 [details]
Proposed patch

> +        try:
> +            (opts, filenames) = getopt.getopt(args, '', flags)
> +        except getopt.GetoptError:
> +            # FIXME: Settle on an error handling approach: come up
> +            #        with a consistent guideline as to when and whether
> +            #        a ValueError should be raised versus calling
> +            #        sys.exit when needing to interrupt execution.
> +            self._exit_with_usage('Invalid arguments.')
> +
> +        extra_flag_values = {}
> +        git_commit = None
> +
> +        for (opt, val) in opts:
> +            if opt == '--help':
> +                self._exit_with_usage()
> +            elif opt == '--output':
> +                output_format = val
> +            elif opt == '--verbose':
> +                verbosity = val
> +            elif opt == '--git-commit':
> +                git_commit = val
> +            elif opt == '--filter':
> +                if not val:
> +                    self._exit_with_categories()
> +                # Prepend the defaults.
> +                filter_rules = filter_rules + self._parse_filter_flag(val)
> +            else:
> +                extra_flag_values[opt] = val

I'm not sure when we can reach here. I think unknown options will make getopt raise an exception. We may just want to raise an exception as taking this branch would be just an error of this script? Also, I'm not sure if we need the argument extra_flags for this function.

> +            self.assertEquals(rule.startswith('-'), True)
> +            self.assertEquals(rule[1:] in style.STYLE_CATEGORIES, True)
> +            self.assertEquals(rule in already_seen, False)

I slightly prefer using assertTrue and assertFalse for these assertions.

> +    def test_defaults(self):
> +        defaults = style.ArgumentDefaults(style.DEFAULT_OUTPUT_FORMAT,
> +                                          style.DEFAULT_VERBOSITY,
> +                                          style.WEBKIT_FILTER_RULES)
> +        parser = style.ArgumentParser(defaults)
> +        parser.parse([]) # works

We may want to check the return value of parser.parse?

[Chris Jerdonek]

(In reply to comment #3)
> > +        try:
> > +            (opts, filenames) = getopt.getopt(args, '', flags)
> > + ...
> > +            else:
> > +                extra_flag_values[opt] = val
> I'm not sure when we can reach here. I think unknown options will make getopt
> raise an exception. We may just want to raise an exception as taking this
> branch would be just an error of this script? Also, I'm not sure if we need the
> argument extra_flags for this function.

Thanks for the comments, Shinichiro.

On the first point, the "extra flag" functionality was there before, and I simply preserved it rather than take it out.  The check-webkit-style file originally used it to let the "git-commit" flag pass through parse_arguments() without cpp_style.py having to know about it.

With this patch we will no longer be using the extra flags, and I doubt we'll need it any longer, but I didn't want to remove functionality someone took time to add.  I can go either way.

The else clause is reachable because earlier in the function, the extra_flag values are added to flags:

> for extra_flag in extra_flags:
>     ...
>     flags.append(extra_flag)

There are some unit tests in style_unittest.py (both succeeding ones and failing ones) that pass extra flag values and exercise the "else" code path.

[Maciej Stachowiak]

Retitling to avoid confusion with patches needing specialized review.

[Eric Seidel (no email)]

Comment on attachment 45541 [details]
Proposed patch

I'm willing to rubber-stamp this change, but Dave Levin would be your best reviewer.  Despite Adam and I being useful for python changes, neither of us have touched this code.

[David Levin]

This is one of three patches that I'm looking at this today... (I was out for vacation).

[Chris Jerdonek]

(In reply to comment #7)
> This is one of three patches that I'm looking at this today... (I was out for
> vacation).

Thanks, guys!  And welcome back, David!

After thinking about Shinichiro's comment on the extra_flags parameter, I'd say we should take it out (either in this report or a later report), and then switch to OptionParser as Eric suggested to me when reviewing a different script:

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

[David Levin]

Comment on attachment 45541 [details]
Proposed patch

Just a few minor things to address.


> Index: WebKitTools/Scripts/modules/style.py

> -def exit_with_categories(display_help=False):
> -    """Exit and print all style categories, along with the default filter.
> +    def __init__(self, output_format, verbosity=1, filter_rules=None,
> +                 git_commit=None, extra_flag_values=None):
> +        if filter_rules is None:
> +            filter_rules = []
> +        if extra_flag_values is None:
> +            extra_flag_values is {}

I think you meant
   extra_flag_values = {}

> +        for extra_flag in extra_flags:
> +            if extra_flag in flags:
> +                raise ValueError('Flag \'%(extra_flag)s is duplicated '
> +                                 'or already supported.' %
> +                                 {'extra_flag': extra_flag })

Extra space after extra_flag

> Index: WebKitTools/Scripts/modules/style_unittest.py
> +    def _create_parser(self, defaults=None):
> +        """"Return an ArgumentParser instance for testing."""
> +        def create_usage(_defaults):
> +            """Return a usage string for testing."""
> +            return "usage";

Get rid of the trailing ";"
(Your C++ programming is showing.)


Also please consider Shinichiro's other comments:

I slightly prefer using assertTrue and assertFalse for these assertions.

> +    def test_defaults(self):
> +        defaults = style.ArgumentDefaults(style.DEFAULT_OUTPUT_FORMAT,
> +                                          style.DEFAULT_VERBOSITY,
> +                                          style.WEBKIT_FILTER_RULES)
> +        parser = style.ArgumentParser(defaults)
> +        parser.parse([]) # works

We may want to check the return value of parser.parse?

[Chris Jerdonek]

(In reply to comment #9)
> > +        def create_usage(_defaults):
> > +            """Return a usage string for testing."""
> > +            return "usage";
> 
> Get rid of the trailing ";"
> (Your C++ programming is showing.)

Thanks, David -- good eye!  And thanks, Shinichiro!

I'm incorporating all of your and Shinichiro's comments.  In regards to checking the return value of parse() when passing default arguments, I'm adding a couple comments there instead of adding an explicit check.  The reason is that if some input value code paths aren't being checked, those checks should really be added to the unit tests that check parse()'s return values.

[Chris Jerdonek]

Created attachment 45954 [details]
Proposed patch 2

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45954 [details] without any errors.

[WebKit Commit Bot]

Comment on attachment 45954 [details]
Proposed patch 2

Clearing flags on attachment: 45954

Committed r52849: <http://trac.webkit.org/changeset/52849>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.