Parse comma-separated multiple values.

Parse comma-separated multiple values.

R=rnystrom@google.com

Committed: https://github.com/dart-lang/args/commit/52fe6a182296ddf705379790b41012d69d5e4ed1

[Bob Nystrom, 2015-03-03 17:36:26]

https://codereview.chromium.org/975463004/diff/20001/lib/src/parser.dart
File lib/src/parser.dart (right):

https://codereview.chromium.org/975463004/diff/20001/lib/src/parser.dart#newc...
lib/src/parser.dart:264: assert(value is String);
Value can only be a string or bool, right? And flags don't allow multiple. Maybe
this would be clearer to have separate setOption() and setFlag() methods that
can be more statically typed. Thoughts?

https://codereview.chromium.org/975463004/diff/20001/lib/src/parser.dart#newc...
lib/src/parser.dart:265: for (var subValue in value.split(",")) {
"subValue" -> "subvalue". Or maybe just "element"?

https://codereview.chromium.org/975463004/diff/20001/lib/src/parser.dart#newc...
lib/src/parser.dart:272: void _validateAllowed(Option option, value) {
Annotate value as String.

https://codereview.chromium.org/975463004/diff/20001/lib/src/parser.dart#newc...
lib/src/parser.dart:275: validate(option.allowed.any((allow) => allow == value),
.contains(value)

?

https://codereview.chromium.org/975463004/diff/20001/test/parse_test.dart
File test/parse_test.dart (right):

https://codereview.chromium.org/975463004/diff/20001/test/parse_test.dart#new...
test/parse_test.dart:319: 
Tests for:

","
"a,,b"
",a"
"a,"

[Sean Eagan, 2015-03-03 20:17:47]

On 2015/03/03 01:17:51, nweiz wrote:

What is the workaround if the individual values can contain commas (such as
regular expressions)?  Should there be a way to opt out?

[Sean Eagan, 2015-03-03 20:21:23]

https://codereview.chromium.org/975463004/diff/20001/lib/src/parser.dart
File lib/src/parser.dart (right):

https://codereview.chromium.org/975463004/diff/20001/lib/src/parser.dart#newc...
lib/src/parser.dart:264: assert(value is String);
On 2015/03/03 17:36:26, Bob Nystrom wrote:
> And flags don't allow multiple.

They don't?  I know some people like to use `-vvv` to represent how verbose they
want the output to be.

[nweiz, 2015-03-03 20:21:48]

Code review changes

[nweiz, 2015-03-03 20:22:18]

https://codereview.chromium.org/975463004/diff/20001/lib/src/parser.dart
File lib/src/parser.dart (right):

https://codereview.chromium.org/975463004/diff/20001/lib/src/parser.dart#newc...
lib/src/parser.dart:264: assert(value is String);
On 2015/03/03 17:36:26, Bob Nystrom wrote:
> Value can only be a string or bool, right? And flags don't allow multiple.
Maybe
> this would be clearer to have separate setOption() and setFlag() methods that
> can be more statically typed. Thoughts?

Done.

https://codereview.chromium.org/975463004/diff/20001/lib/src/parser.dart#newc...
lib/src/parser.dart:265: for (var subValue in value.split(",")) {
On 2015/03/03 17:36:26, Bob Nystrom wrote:
> "subValue" -> "subvalue". Or maybe just "element"?

Done.

https://codereview.chromium.org/975463004/diff/20001/lib/src/parser.dart#newc...
lib/src/parser.dart:272: void _validateAllowed(Option option, value) {
On 2015/03/03 17:36:26, Bob Nystrom wrote:
> Annotate value as String.

Done.

https://codereview.chromium.org/975463004/diff/20001/lib/src/parser.dart#newc...
lib/src/parser.dart:275: validate(option.allowed.any((allow) => allow == value),
On 2015/03/03 17:36:26, Bob Nystrom wrote:
> .contains(value)
> 
> ?

Done.

https://codereview.chromium.org/975463004/diff/20001/test/parse_test.dart
File test/parse_test.dart (right):

https://codereview.chromium.org/975463004/diff/20001/test/parse_test.dart#new...
test/parse_test.dart:319: 
On 2015/03/03 17:36:26, Bob Nystrom wrote:
> Tests for:
> 
> ","
> "a,,b"
> ",a"
> "a,"

I don't think these should throw... multi-value options allow the empty string
in general, so I don't see why they shouldn't allow it with this syntax as well.
I'll add a test case above.

[nweiz, 2015-03-03 20:39:40]

On 2015/03/03 20:17:47, Sean Eagan wrote:
> On 2015/03/03 01:17:51, nweiz wrote:
> 
> What is the workaround if the individual values can contain commas (such as
> regular expressions)?  Should there be a way to opt out?

I suppose there could be a splitCommas flag that defaults to true. Bob, what do
you think?

[Bob Nystrom, 2015-03-03 21:19:16]

On 2015/03/03 20:39:40, nweiz wrote:
> On 2015/03/03 20:17:47, Sean Eagan wrote:
> > On 2015/03/03 01:17:51, nweiz wrote:
> > 
> > What is the workaround if the individual values can contain commas (such as
> > regular expressions)?  Should there be a way to opt out?
> 
> I suppose there could be a splitCommas flag that defaults to true. Bob, what
do
> you think?

Yeah, I mentioned something along those lines yesterday. If we default it to
false, it means this wouldn't be a breaking change, which is probably a good
thing.

- bob

https://chromiumcodereview.appspot.com/975463004/diff/40001/lib/src/parser.dart
File lib/src/parser.dart (right):

https://chromiumcodereview.appspot.com/975463004/diff/40001/lib/src/parser.da...
lib/src/parser.dart:277: results[option.name] = value;
Ha, now that I see how little is left here, may as well just inline this in the
few places it's used and delete this method.

[nweiz, 2015-03-03 21:54:48]

Even if we add the flag, I think commas should be parsed by default. Wanting
commas in property values is an edge case.

[Bob Nystrom, 2015-03-03 23:42:56]

On 2015/03/03 21:54:48, nweiz wrote:
> Even if we add the flag, I think commas should be parsed by default. Wanting
> commas in property values is an edge case.

I agree it's the minority case, but defaulting to the current behavior maximizes
stability. It's consistent with other options we added:

- negatable
- allow trailing args

In both cases, the default is not what the user wants most of the time but kept
with the old behavior.

[nweiz, 2015-03-03 23:58:05]

On 2015/03/03 23:42:56, Bob Nystrom wrote:
> On 2015/03/03 21:54:48, nweiz wrote:
> > Even if we add the flag, I think commas should be parsed by default. Wanting
> > commas in property values is an edge case.
> 
> I agree it's the minority case, but defaulting to the current behavior
maximizes
> stability. It's consistent with other options we added:
> 
> - negatable
> - allow trailing args
> 
> In both cases, the default is not what the user wants most of the time but
kept
> with the old behavior.

Those are options whose reversal could potentially invalidate a large swath of
existing code, though. It's unlikely that there's much existing code that relies
on comma-separated multivalues, and everything else gets a bonus for free.

If you still want to make this non-breaking, can we figure out a way to release
a breaking change that fixes all the defaults sooner than later?

[Bob Nystrom, 2015-03-04 00:16:48]

On 2015/03/03 23:58:05, nweiz wrote:
> On 2015/03/03 23:42:56, Bob Nystrom wrote:
> > On 2015/03/03 21:54:48, nweiz wrote:
> > > Even if we add the flag, I think commas should be parsed by default.
Wanting
> > > commas in property values is an edge case.
> > 
> > I agree it's the minority case, but defaulting to the current behavior
> maximizes
> > stability. It's consistent with other options we added:
> > 
> > - negatable
> > - allow trailing args
> > 
> > In both cases, the default is not what the user wants most of the time but
> kept
> > with the old behavior.
> 
> Those are options whose reversal could potentially invalidate a large swath of
> existing code, though. It's unlikely that there's much existing code that
relies
> on comma-separated multivalues, and everything else gets a bonus for free.
> 
> If you still want to make this non-breaking, can we figure out a way to
release
> a breaking change that fixes all the defaults sooner than later?

OK, per our discussion, kick it out with the right default and mark it a
breaking change.

- bob

[nweiz, 2015-03-04 01:52:45]

Added a splitCommas option; PTAL.

[Bob Nystrom, 2015-03-04 22:01:37]

https://chromiumcodereview.appspot.com/975463004/diff/60001/README.md
File README.md (right):

https://chromiumcodereview.appspot.com/975463004/diff/60001/README.md#newcode192
README.md:192: By default, multiple options' values may also be separated with
commas:
This reads a bit ambiguously to me (it could mean multiple different options).
How about: "values for a multi-valued option"?

https://chromiumcodereview.appspot.com/975463004/diff/60001/lib/src/arg_parse...
File lib/src/arg_parser.dart (right):

https://chromiumcodereview.appspot.com/975463004/diff/60001/lib/src/arg_parse...
lib/src/arg_parser.dart:86: throw new ArgumentError('splitCommas may not be set
if allowMultiple is '
If you wrap before the string literal, you don't have to split the string
itself.

https://chromiumcodereview.appspot.com/975463004/diff/60001/lib/src/option.dart
File lib/src/option.dart (right):

https://chromiumcodereview.appspot.com/975463004/diff/60001/lib/src/option.da...
lib/src/option.dart:57: this.splitCommas = splitCommas == null
Can you add a little comment explaining this?

https://chromiumcodereview.appspot.com/975463004/diff/60001/lib/src/parser.dart
File lib/src/parser.dart (right):

https://chromiumcodereview.appspot.com/975463004/diff/60001/lib/src/parser.da...
lib/src/parser.dart:269: for (var element in value.split(",")) {
What do you think about trimming whitespace?

[nweiz, 2015-03-04 22:05:35]

Code review changes

[nweiz, 2015-03-04 22:05:41]

https://chromiumcodereview.appspot.com/975463004/diff/60001/README.md
File README.md (right):

https://chromiumcodereview.appspot.com/975463004/diff/60001/README.md#newcode192
README.md:192: By default, multiple options' values may also be separated with
commas:
On 2015/03/04 22:01:37, Bob Nystrom wrote:
> This reads a bit ambiguously to me (it could mean multiple different options).
> How about: "values for a multi-valued option"?

Done.

https://chromiumcodereview.appspot.com/975463004/diff/60001/lib/src/arg_parse...
File lib/src/arg_parser.dart (right):

https://chromiumcodereview.appspot.com/975463004/diff/60001/lib/src/arg_parse...
lib/src/arg_parser.dart:86: throw new ArgumentError('splitCommas may not be set
if allowMultiple is '
On 2015/03/04 22:01:37, Bob Nystrom wrote:
> If you wrap before the string literal, you don't have to split the string
> itself.

Done.

https://chromiumcodereview.appspot.com/975463004/diff/60001/lib/src/option.dart
File lib/src/option.dart (right):

https://chromiumcodereview.appspot.com/975463004/diff/60001/lib/src/option.da...
lib/src/option.dart:57: this.splitCommas = splitCommas == null
On 2015/03/04 22:01:37, Bob Nystrom wrote:
> Can you add a little comment explaining this?

Done.

https://chromiumcodereview.appspot.com/975463004/diff/60001/lib/src/parser.dart
File lib/src/parser.dart (right):

https://chromiumcodereview.appspot.com/975463004/diff/60001/lib/src/parser.da...
lib/src/parser.dart:269: for (var element in value.split(",")) {
On 2015/03/04 22:01:37, Bob Nystrom wrote:
> What do you think about trimming whitespace?

Users really have to go out of their way to add whitespace on the shell; I think
it's likely to do as much harm as good.

[Bob Nystrom, 2015-03-04 22:27:31]

lgtm

[Sean Eagan, 2015-03-04 22:28:03]

https://chromiumcodereview.appspot.com/975463004/diff/80001/lib/src/arg_parse...
File lib/src/arg_parser.dart (right):

https://chromiumcodereview.appspot.com/975463004/diff/80001/lib/src/arg_parse...
lib/src/arg_parser.dart:87: 'splitCommas may not be set if allowMultiple is
false.');
Why not?  Seems like it would be reasonable that someone would want to allow
e.g.
`--foo=a,b,c`, but not `--foo=a,b --foo=c`.  In fact the latter seems kind of
useless.

[nweiz, 2015-03-04 22:39:55]

https://chromiumcodereview.appspot.com/975463004/diff/80001/lib/src/arg_parse...
File lib/src/arg_parser.dart (right):

https://chromiumcodereview.appspot.com/975463004/diff/80001/lib/src/arg_parse...
lib/src/arg_parser.dart:87: 'splitCommas may not be set if allowMultiple is
false.');
On 2015/03/04 22:28:03, Sean Eagan wrote:
> Why not?  Seems like it would be reasonable that someone would want to allow
> e.g.
> `--foo=a,b,c`, but not `--foo=a,b --foo=c`.  In fact the latter seems kind of
> useless.

Why would you want to forbid the latter? That provides no additional
expressiveness, and is contrary to how many existing command-line tools work.

[nweiz, 2015-03-05 01:54:36]

Committed patchset #5 (id:80001) manually as
52fe6a182296ddf705379790b41012d69d5e4ed1 (presubmit successful).