Created attachment 45541 [details] Proposed patch This is part 2 of the patch originally proposed in the bug report mentioned in the first comment.

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

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?

(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.

Retitling to avoid confusion with patches needing specialized review.

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.

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

(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 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?

(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.

Created attachment 45954 [details] Proposed patch 2

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

Comment on attachment 45954 [details] Proposed patch 2 Clearing flags on attachment: 45954 Committed r52849: <http://trac.webkit.org/changeset/52849>

All reviewed patches have been landed. Closing bug.