Bug 32966 - check-webkit-style: Refactor style.parse_arguments so that it has no global variable side effects.
Summary: check-webkit-style: Refactor style.parse_arguments so that it has no global v...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P4 Normal
Assignee: Chris Jerdonek
URL:
Keywords:
Depends on: 32592
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-27 11:51 PST by Chris Jerdonek
Modified: 2010-01-05 22:38 PST (History)
9 users (show)

See Also:


Attachments
Proposed patch (39.33 KB, patch)
2009-12-27 19:54 PST, Chris Jerdonek
levin: review-
levin: commit-queue-
Details | Formatted Diff | Diff
Proposed patch 2 (40.52 KB, patch)
2010-01-05 20:49 PST, Chris Jerdonek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Jerdonek 2009-12-27 11:51:33 PST
Continuation of the following report (for Patch 2 of 2):

https://bugs.webkit.org/show_bug.cgi?id=32592
Comment 1 Chris Jerdonek 2009-12-27 19:54:55 PST
Created attachment 45541 [details]
Proposed patch

This is part 2 of the patch originally proposed in the bug report mentioned in the first comment.
Comment 2 WebKit Review Bot 2009-12-27 19:59:30 PST
style-queue ran check-webkit-style on attachment 45541 [details] without any errors.
Comment 3 Shinichiro Hamaji 2009-12-27 23:32:39 PST
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?
Comment 4 Chris Jerdonek 2009-12-28 00:03:13 PST
(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.
Comment 5 Maciej Stachowiak 2009-12-28 17:33:55 PST
Retitling to avoid confusion with patches needing specialized review.
Comment 6 Eric Seidel 2010-01-05 13:26:42 PST
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.
Comment 7 David Levin 2010-01-05 13:28:59 PST
This is one of three patches that I'm looking at this today... (I was out for vacation).
Comment 8 Chris Jerdonek 2010-01-05 13:41:14 PST
(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
Comment 9 David Levin 2010-01-05 16:16:46 PST
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?
Comment 10 Chris Jerdonek 2010-01-05 19:23:51 PST
(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.
Comment 11 Chris Jerdonek 2010-01-05 20:49:23 PST
Created attachment 45954 [details]
Proposed patch 2
Comment 12 WebKit Review Bot 2010-01-05 20:52:09 PST
style-queue ran check-webkit-style on attachment 45954 [details] without any errors.
Comment 13 WebKit Commit Bot 2010-01-05 22:38:04 PST
Comment on attachment 45954 [details]
Proposed patch 2

Clearing flags on attachment: 45954

Committed r52849: <http://trac.webkit.org/changeset/52849>
Comment 14 WebKit Commit Bot 2010-01-05 22:38:11 PST
All reviewed patches have been landed.  Closing bug.