[webkit-reviews] review denied: [Bug 32966] check-webkit-style: Refactor style.parse_arguments so that it has no global variable side effects. : [Attachment 45541] Proposed patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Chris Jerdonek
<chris.jerdonek at gmail.com>'s request for review:
Bug 32966: check-webkit-style: Refactor style.parse_arguments so that it has no
global variable side effects.
https://bugs.webkit.org/show_bug.cgi?id=32966

Attachment 45541: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=45541&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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?