Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12545013:
Added the continueParsing option to ArgParser. (Closed)
|
Created:
7 years, 9 months ago by Andrei Mouravski Modified:
7 years, 6 months ago Reviewers:
Bob Nystrom CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionAdded the continueParsing option to ArgParser.
R=rnystrom@google.com
Committed: https://code.google.com/p/dart/source/detail?r=24366
Patch Set 1 #Patch Set 2 : #
Total comments: 1
Patch Set 3 : #Patch Set 4 : Style + commentary. #
Total comments: 5
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : Fixed up documentation and the like. #
Total comments: 28
Patch Set 8 : Rebased #Patch Set 9 : 12 changes. #
Total comments: 16
Patch Set 10 : Removed a lot of unnecesary code. #
Total comments: 6
Patch Set 11 : Done #
Messages
Total messages: 16 (0 generated)
Coincidentally, I already have a patch out that implements this: https://codereview.chromium.org/11824044/ The main difference is that the older patch doesn't make this an option, it just makes it the default behavior. I haven't submitted it because it will break pub with the way pub currently uses args. Once pub is using subcommands from the args package, I can land this. (Or I suppose like your patch, I could make it an optional feature, but I do like to minimize the amount of configuration required to use something.)
I think it should be switched to default the other way, but the option is definitely useful for when you want to pass a long command line to another tool. Something like: dart start_server -xyz some_other_command.py --otherCommandFlag=foo otherCommandArg Here you'd want every flag past -xyz to go to some_other_command.py Since you designed the package, I leave it up to you. I'd like one or the other, though, to make using dartdoc easier.
Ping. Any thoughts?
Ping.
This looks mostly fine, but you'll have to do some more work to make it interact correctly with commands. We should talk about how that should work. https://codereview.chromium.org/12545013/diff/2001/pkg/args/test/parse_test.dart File pkg/args/test/parse_test.dart (right): https://codereview.chromium.org/12545013/diff/2001/pkg/args/test/parse_test.d... pkg/args/test/parse_test.dart:397: '--tweet', 'devrel', '--neigh', 'end' ]); Indent +2.
PTAL.
https://codereview.chromium.org/12545013/diff/16001/pkg/args/test/parse_test.... File pkg/args/test/parse_test.dart (right): https://codereview.chromium.org/12545013/diff/16001/pkg/args/test/parse_test.... pkg/args/test/parse_test.dart:392: parser.addFlag('neigh'); Why add neigh to this test? https://codereview.chromium.org/12545013/diff/16001/pkg/args/test/parse_test.... pkg/args/test/parse_test.dart:415: '--tweet', 'devrel', '--neigh', 'end' ], This test is pretty complicated. How about simplifying it to just the one thing being tested, like: parser.addFlag('woof'); parser.parse(['notoption', 'woof']); https://codereview.chromium.org/12545013/diff/16001/pkg/args/test/parse_test.... pkg/args/test/parse_test.dart:423: Add a test for what happens if continueParsing is true and an argument that looks like an option but isn't ('--notopt') is encountered. https://codereview.chromium.org/12545013/diff/16001/pkg/args/test/parse_test.... pkg/args/test/parse_test.dart:434: ..addFlag('neigh'); We should use a consistent style here. How about just ditching the cascades? https://codereview.chromium.org/12545013/diff/16001/pkg/args/test/parse_test.... pkg/args/test/parse_test.dart:447: expect(results.command.command.rest, orderedEquals(['meow', 'not', 'end'])); This test is big and hard to read. Split it into a couple of smaller ones: 1. Test that skipped arguments are passed to the leafmost command. 2. Test parsed options are given to the leafmost command when both a command and its parent define the same option. Also, I would move these tests to command_test.dart
PTAL. This CL depends on https://codereview.chromium.org/17333004
https://codereview.chromium.org/12545013/diff/32001/pkg/args/lib/args.dart File pkg/args/lib/args.dart (right): https://codereview.chromium.org/12545013/diff/32001/pkg/args/lib/args.dart#ne... pkg/args/lib/args.dart:351: * If [parseAllOptions] is set, the parser will continue parsing even after it How about "allowTrailingOptions"? https://codereview.chromium.org/12545013/diff/32001/pkg/args/lib/args.dart#ne... pkg/args/lib/args.dart:357: * leafmost command. "any remaining arguments are passed to the leafmost command" -> "all remaining arguments, even ones that look like options, are not parsed". https://codereview.chromium.org/12545013/diff/32001/pkg/args/lib/args.dart#ne... pkg/args/lib/args.dart:362: ArgResults parse(List<String> args, {bool parseAllOptions: false}) => Instead of using a default here, initialize the default in the body: if (parseAllOptions == null) parseAllOptions = false; That handles the case where a user does parse(parseAllOptions: null) https://codereview.chromium.org/12545013/diff/32001/pkg/args/lib/src/parser.dart File pkg/args/lib/src/parser.dart (right): https://codereview.chromium.org/12545013/diff/32001/pkg/args/lib/src/parser.d... pkg/args/lib/src/parser.dart:27: * throw a [FormatException], regardless of the value of [parseAllOptions]. Redundant with ArgParser docs. Remove, I think. https://codereview.chromium.org/12545013/diff/32001/pkg/args/lib/src/parser.d... pkg/args/lib/src/parser.dart:59: : this.rest = rest == null ? [] : rest; How about having the field be: final rest = <String>[]; and then in the constructor body, do if (rest != null) this.rest.addAll(rest); That way the caller doesn't have to copy. https://codereview.chromium.org/12545013/diff/32001/pkg/args/lib/src/parser.d... pkg/args/lib/src/parser.dart:107: 'Could not find an option or flag "${args[0]}".'); Why is this throw needed? Shouldn't it have already thrown? https://codereview.chromium.org/12545013/diff/32001/pkg/args/lib/src/parser.d... pkg/args/lib/src/parser.dart:304: * Throws a [FormatException] exception otherwise. This documents *what* it does, but not *why* it does that. https://codereview.chromium.org/12545013/diff/32001/pkg/args/lib/src/parser.d... pkg/args/lib/src/parser.dart:310: return false; What's this case for? If we can't parse it, shouldn't it always fail? We only get here if it's an option. https://codereview.chromium.org/12545013/diff/32001/pkg/args/test/parse_all_t... File pkg/args/test/parse_all_test.dart (right): https://codereview.chromium.org/12545013/diff/32001/pkg/args/test/parse_all_t... pkg/args/test/parse_all_test.dart:11: group('ArgParser.parse()', () { "ArgParser.parse(parseAllOptions: true)" https://codereview.chromium.org/12545013/diff/32001/pkg/args/test/parse_all_t... pkg/args/test/parse_all_test.dart:27: expect(results.rest, orderedEquals(args)); Don't other tests cover this? https://codereview.chromium.org/12545013/diff/32001/pkg/args/test/parse_all_t... pkg/args/test/parse_all_test.dart:88: parser.parse(args, parseAllOptions: true); I'd probably just inline this in the tests to make them a bit easier to read. https://codereview.chromium.org/12545013/diff/32001/pkg/args/test/parse_all_t... pkg/args/test/parse_all_test.dart:90: expectThrows(ArgParser parser, List<String> args, {parseAllOptions: false}) => You're always passing true for parseAllOptions. Just remove the param and hardcode that.
PTAL. https://codereview.chromium.org/12545013/diff/32001/pkg/args/lib/args.dart File pkg/args/lib/args.dart (right): https://codereview.chromium.org/12545013/diff/32001/pkg/args/lib/args.dart#ne... pkg/args/lib/args.dart:351: * If [parseAllOptions] is set, the parser will continue parsing even after it On 2013/06/20 00:33:36, Bob Nystrom wrote: > How about "allowTrailingOptions"? Done. https://codereview.chromium.org/12545013/diff/32001/pkg/args/lib/args.dart#ne... pkg/args/lib/args.dart:357: * leafmost command. On 2013/06/20 00:33:36, Bob Nystrom wrote: > "any remaining arguments are passed to the leafmost command" -> "all remaining > arguments, even ones that look like options, are not parsed". Done. https://codereview.chromium.org/12545013/diff/32001/pkg/args/lib/args.dart#ne... pkg/args/lib/args.dart:362: ArgResults parse(List<String> args, {bool parseAllOptions: false}) => On 2013/06/20 00:33:36, Bob Nystrom wrote: > Instead of using a default here, initialize the default in the body: > > if (parseAllOptions == null) parseAllOptions = false; > > That handles the case where a user does parse(parseAllOptions: null) Done. https://codereview.chromium.org/12545013/diff/32001/pkg/args/lib/src/parser.dart File pkg/args/lib/src/parser.dart (right): https://codereview.chromium.org/12545013/diff/32001/pkg/args/lib/src/parser.d... pkg/args/lib/src/parser.dart:27: * throw a [FormatException], regardless of the value of [parseAllOptions]. On 2013/06/20 00:33:36, Bob Nystrom wrote: > Redundant with ArgParser docs. Remove, I think. Done. https://codereview.chromium.org/12545013/diff/32001/pkg/args/lib/src/parser.d... pkg/args/lib/src/parser.dart:59: : this.rest = rest == null ? [] : rest; On 2013/06/20 00:33:36, Bob Nystrom wrote: > How about having the field be: > > final rest = <String>[]; > > and then in the constructor body, do if (rest != null) this.rest.addAll(rest); > > That way the caller doesn't have to copy. Done. https://codereview.chromium.org/12545013/diff/32001/pkg/args/lib/src/parser.d... pkg/args/lib/src/parser.dart:107: 'Could not find an option or flag "${args[0]}".'); On 2013/06/20 00:33:36, Bob Nystrom wrote: > Why is this throw needed? Shouldn't it have already thrown? Because I'm more lenient inside the parse functions and this catches things that look like options but aren't. https://codereview.chromium.org/12545013/diff/32001/pkg/args/lib/src/parser.d... pkg/args/lib/src/parser.dart:304: * Throws a [FormatException] exception otherwise. On 2013/06/20 00:33:36, Bob Nystrom wrote: > This documents *what* it does, but not *why* it does that. Done. https://codereview.chromium.org/12545013/diff/32001/pkg/args/lib/src/parser.d... pkg/args/lib/src/parser.dart:310: return false; On 2013/06/20 00:33:36, Bob Nystrom wrote: > What's this case for? If we can't parse it, shouldn't it always fail? We only > get here if it's an option. It's to fall through when parsing. https://codereview.chromium.org/12545013/diff/32001/pkg/args/test/parse_all_t... File pkg/args/test/parse_all_test.dart (right): https://codereview.chromium.org/12545013/diff/32001/pkg/args/test/parse_all_t... pkg/args/test/parse_all_test.dart:11: group('ArgParser.parse()', () { On 2013/06/20 00:33:36, Bob Nystrom wrote: > "ArgParser.parse(parseAllOptions: true)" Done. https://codereview.chromium.org/12545013/diff/32001/pkg/args/test/parse_all_t... pkg/args/test/parse_all_test.dart:27: expect(results.rest, orderedEquals(args)); On 2013/06/20 00:33:36, Bob Nystrom wrote: > Don't other tests cover this? Not quite. https://codereview.chromium.org/12545013/diff/32001/pkg/args/test/parse_all_t... pkg/args/test/parse_all_test.dart:88: parser.parse(args, parseAllOptions: true); On 2013/06/20 00:33:36, Bob Nystrom wrote: > I'd probably just inline this in the tests to make them a bit easier to read. Done. https://codereview.chromium.org/12545013/diff/32001/pkg/args/test/parse_all_t... pkg/args/test/parse_all_test.dart:90: expectThrows(ArgParser parser, List<String> args, {parseAllOptions: false}) => On 2013/06/20 00:33:36, Bob Nystrom wrote: > You're always passing true for parseAllOptions. Just remove the param and > hardcode that. Done.
https://codereview.chromium.org/12545013/diff/32001/pkg/args/lib/src/parser.dart File pkg/args/lib/src/parser.dart (right): https://codereview.chromium.org/12545013/diff/32001/pkg/args/lib/src/parser.d... pkg/args/lib/src/parser.dart:107: 'Could not find an option or flag "${args[0]}".'); On 2013/06/22 00:54:02, Andrei Mouravski wrote: > On 2013/06/20 00:33:36, Bob Nystrom wrote: > > Why is this throw needed? Shouldn't it have already thrown? > > Because I'm more lenient inside the parse functions *Why* are you being more lenient? Those functions should immediately throw if they match their regex but can't find an appropriate option. > and this catches things that look like options but aren't. Why aren't those already caught like they were before? https://codereview.chromium.org/12545013/diff/32001/pkg/args/test/parse_all_t... File pkg/args/test/parse_all_test.dart (right): https://codereview.chromium.org/12545013/diff/32001/pkg/args/test/parse_all_t... pkg/args/test/parse_all_test.dart:27: expect(results.rest, orderedEquals(args)); On 2013/06/22 00:54:02, Andrei Mouravski wrote: > On 2013/06/20 00:33:36, Bob Nystrom wrote: > > Don't other tests cover this? > > Not quite. Can you be more specific? There are existing comprehensive tests for when parseAllOptions is false. Why do we need more tests for that? https://codereview.chromium.org/12545013/diff/41001/pkg/args/lib/args.dart File pkg/args/lib/args.dart (right): https://codereview.chromium.org/12545013/diff/41001/pkg/args/lib/args.dart#ne... pkg/args/lib/args.dart:353: * This allows options to be specified after command parameters. "command parameters" -> "regular arguments". https://codereview.chromium.org/12545013/diff/41001/pkg/args/lib/args.dart#ne... pkg/args/lib/args.dart:357: * even those that look like options are passed to the leafmost command. "leafmost" -> "innermost". https://codereview.chromium.org/12545013/diff/41001/pkg/args/lib/args.dart#ne... pkg/args/lib/args.dart:361: * of [allowTrailingOptions]. This is a bit confusing. It isn't clear that this is *not* the case for trailing arguments after a non-option one when allowTrailingOptions is false. Maybe just remove the paragraph? https://codereview.chromium.org/12545013/diff/41001/pkg/args/lib/args.dart#ne... pkg/args/lib/args.dart:365: return new Parser(null, this, args.toList(), allowTrailingOptions).parse(); It should be a named argument here too. https://codereview.chromium.org/12545013/diff/41001/pkg/args/lib/src/parser.dart File pkg/args/lib/src/parser.dart (right): https://codereview.chromium.org/12545013/diff/41001/pkg/args/lib/src/parser.d... pkg/args/lib/src/parser.dart:31: /** If `true`, parser will continue after it sees a non-option argument. */ "parser" -> "parsing" "it sees" -> "" https://codereview.chromium.org/12545013/diff/41001/pkg/args/lib/src/parser.d... pkg/args/lib/src/parser.dart:47: [this.allowTrailingOptions = false, this.parent, rest]) { Ugh. Boolean parameters should be named. Since you can't mix named and positional params, I would make parent and rest non-optional. https://codereview.chromium.org/12545013/diff/41001/pkg/args/lib/src/parser.d... pkg/args/lib/src/parser.dart:83: allowTrailingOptions, this, rest.toList()); .toList() isn't needed. https://codereview.chromium.org/12545013/diff/41001/pkg/args/lib/src/parser.d... pkg/args/lib/src/parser.dart:119: rest.addAll(args.toList()); Don't need .toList() here.
PTAL. https://codereview.chromium.org/12545013/diff/32001/pkg/args/lib/src/parser.dart File pkg/args/lib/src/parser.dart (right): https://codereview.chromium.org/12545013/diff/32001/pkg/args/lib/src/parser.d... pkg/args/lib/src/parser.dart:107: 'Could not find an option or flag "${args[0]}".'); On 2013/06/24 15:53:20, Bob Nystrom wrote: > On 2013/06/22 00:54:02, Andrei Mouravski wrote: > > On 2013/06/20 00:33:36, Bob Nystrom wrote: > > > Why is this throw needed? Shouldn't it have already thrown? > > > > Because I'm more lenient inside the parse functions > > *Why* are you being more lenient? Those functions should immediately throw if > they match their regex but can't find an appropriate option. > > > and this catches things that look like options but aren't. > > Why aren't those already caught like they were before? Done. https://codereview.chromium.org/12545013/diff/32001/pkg/args/test/parse_all_t... File pkg/args/test/parse_all_test.dart (right): https://codereview.chromium.org/12545013/diff/32001/pkg/args/test/parse_all_t... pkg/args/test/parse_all_test.dart:27: expect(results.rest, orderedEquals(args)); On 2013/06/24 15:53:20, Bob Nystrom wrote: > On 2013/06/22 00:54:02, Andrei Mouravski wrote: > > On 2013/06/20 00:33:36, Bob Nystrom wrote: > > > Don't other tests cover this? > > > > Not quite. > > Can you be more specific? There are existing comprehensive tests for when > parseAllOptions is false. Why do we need more tests for that? Done. https://codereview.chromium.org/12545013/diff/41001/pkg/args/lib/args.dart File pkg/args/lib/args.dart (right): https://codereview.chromium.org/12545013/diff/41001/pkg/args/lib/args.dart#ne... pkg/args/lib/args.dart:353: * This allows options to be specified after command parameters. On 2013/06/24 15:53:20, Bob Nystrom wrote: > "command parameters" -> "regular arguments". Done. https://codereview.chromium.org/12545013/diff/41001/pkg/args/lib/args.dart#ne... pkg/args/lib/args.dart:357: * even those that look like options are passed to the leafmost command. On 2013/06/24 15:53:20, Bob Nystrom wrote: > "leafmost" -> "innermost". Done. https://codereview.chromium.org/12545013/diff/41001/pkg/args/lib/args.dart#ne... pkg/args/lib/args.dart:361: * of [allowTrailingOptions]. On 2013/06/24 15:53:20, Bob Nystrom wrote: > This is a bit confusing. It isn't clear that this is *not* the case for trailing > arguments after a non-option one when allowTrailingOptions is false. Maybe just > remove the paragraph? Done. https://codereview.chromium.org/12545013/diff/41001/pkg/args/lib/args.dart#ne... pkg/args/lib/args.dart:365: return new Parser(null, this, args.toList(), allowTrailingOptions).parse(); On 2013/06/24 15:53:20, Bob Nystrom wrote: > It should be a named argument here too. Done. https://codereview.chromium.org/12545013/diff/41001/pkg/args/lib/src/parser.dart File pkg/args/lib/src/parser.dart (right): https://codereview.chromium.org/12545013/diff/41001/pkg/args/lib/src/parser.d... pkg/args/lib/src/parser.dart:31: /** If `true`, parser will continue after it sees a non-option argument. */ On 2013/06/24 15:53:20, Bob Nystrom wrote: > "parser" -> "parsing" > "it sees" -> "" Done. https://codereview.chromium.org/12545013/diff/41001/pkg/args/lib/src/parser.d... pkg/args/lib/src/parser.dart:47: [this.allowTrailingOptions = false, this.parent, rest]) { On 2013/06/24 15:53:20, Bob Nystrom wrote: > Ugh. Boolean parameters should be named. Since you can't mix named and > positional params, I would make parent and rest non-optional. Done. https://codereview.chromium.org/12545013/diff/41001/pkg/args/lib/src/parser.d... pkg/args/lib/src/parser.dart:83: allowTrailingOptions, this, rest.toList()); On 2013/06/24 15:53:20, Bob Nystrom wrote: > .toList() isn't needed. Done. https://codereview.chromium.org/12545013/diff/41001/pkg/args/lib/src/parser.d... pkg/args/lib/src/parser.dart:119: rest.addAll(args.toList()); On 2013/06/24 15:53:20, Bob Nystrom wrote: > Don't need .toList() here. Done.
A couple more changes, then LGTM. https://codereview.chromium.org/12545013/diff/55001/pkg/args/lib/src/parser.dart File pkg/args/lib/src/parser.dart (right): https://codereview.chromium.org/12545013/diff/55001/pkg/args/lib/src/parser.d... pkg/args/lib/src/parser.dart:41: List<String> rest = <String>[]; Make this final and lose the type annotation. https://codereview.chromium.org/12545013/diff/55001/pkg/args/lib/src/parser.d... pkg/args/lib/src/parser.dart:93: if (isCurrentArgAnOption) { This isn't needed any more. https://codereview.chromium.org/12545013/diff/55001/pkg/args/lib/src/parser.d... pkg/args/lib/src/parser.dart:141: (re) => re.firstMatch(current) != null); You can ditch this.
Thanks! Submitting. https://codereview.chromium.org/12545013/diff/55001/pkg/args/lib/src/parser.dart File pkg/args/lib/src/parser.dart (right): https://codereview.chromium.org/12545013/diff/55001/pkg/args/lib/src/parser.d... pkg/args/lib/src/parser.dart:41: List<String> rest = <String>[]; On 2013/06/24 20:49:29, Bob Nystrom wrote: > Make this final and lose the type annotation. Done. https://codereview.chromium.org/12545013/diff/55001/pkg/args/lib/src/parser.d... pkg/args/lib/src/parser.dart:93: if (isCurrentArgAnOption) { On 2013/06/24 20:49:29, Bob Nystrom wrote: > This isn't needed any more. Done. https://codereview.chromium.org/12545013/diff/55001/pkg/args/lib/src/parser.d... pkg/args/lib/src/parser.dart:141: (re) => re.firstMatch(current) != null); On 2013/06/24 20:49:29, Bob Nystrom wrote: > You can ditch this. Done.
Message was sent while issue was closed.
Committed patchset #11 manually as r24366 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
