Add support for separators.

Add support for separators.

R=rnystrom@google.com

Committed: https://github.com/dart-lang/args/commit/8969a98dc94cda4aa4391499bebb697ecbffe04f

[kevmoo, 2015-05-21 02:44:27]

kevmoo@google.com changed reviewers:
+ kevmoo@google.com

[kevmoo, 2015-05-21 02:44:28]

dbc

https://codereview.chromium.org/1145413002/diff/1/example/test_runner.dart
File example/test_runner.dart (right):

https://codereview.chromium.org/1145413002/diff/1/example/test_runner.dart#ne...
example/test_runner.dart:17: parser.addSeparator('===== Platform');
Would it be worth while to just standardize on this indent model?

Perhaps w/ a named argument to ArgParser ctor?

Convention over config, etc etc

[Bob Nystrom, 2015-05-21 16:43:16]

https://codereview.chromium.org/1145413002/diff/1/lib/src/arg_parser.dart
File lib/src/arg_parser.dart (right):

https://codereview.chromium.org/1145413002/diff/1/lib/src/arg_parser.dart#new...
lib/src/arg_parser.dart:26: final List optionsAndSeparators;
This is kind of gross.

It might be time to consider creating a real class hierarchy for Option:

class Option {}
class SingleOption extends Option {}
class MultipleOption extends Option {}
class Flag extends Option {}
class Separator extends Option {}

Then call this orderedOptions, or just not expose it publicly at all.

https://codereview.chromium.org/1145413002/diff/1/lib/src/arg_parser.dart#new...
lib/src/arg_parser.dart:52: this.optionsAndSeparators = new
UnmodifiableListView(optionsAndSeparators),
Long line.

https://codereview.chromium.org/1145413002/diff/1/lib/src/arg_parser.dart#new...
lib/src/arg_parser.dart:132: /// together visually.
This paragraph is kind of choppy. How about just:

"In the usage text for the parser, this will appear between any options added
before this call and ones added after it."

https://codereview.chromium.org/1145413002/diff/1/lib/src/usage.dart
File lib/src/usage.dart (right):

https://codereview.chromium.org/1145413002/diff/1/lib/src/usage.dart#newcode74
lib/src/usage.dart:74: var option = optionOrSeparator;
Do you need an "as Option" here?

[nweiz, 2015-05-21 19:22:24]

Code review changes

[nweiz, 2015-05-21 19:22:27]

https://codereview.chromium.org/1145413002/diff/1/example/test_runner.dart
File example/test_runner.dart (right):

https://codereview.chromium.org/1145413002/diff/1/example/test_runner.dart#ne...
example/test_runner.dart:17: parser.addSeparator('===== Platform');
On 2015/05/21 02:44:27, kevmoo wrote:
> Would it be worth while to just standardize on this indent model?
> 
> Perhaps w/ a named argument to ArgParser ctor?
> 
> Convention over config, etc etc

I don't think so. It's really easy to add manually, and I don't want to force
users into one particular style when it would be so easy not to do so.

https://codereview.chromium.org/1145413002/diff/1/lib/src/arg_parser.dart
File lib/src/arg_parser.dart (right):

https://codereview.chromium.org/1145413002/diff/1/lib/src/arg_parser.dart#new...
lib/src/arg_parser.dart:26: final List optionsAndSeparators;
On 2015/05/21 16:43:16, Bob Nystrom wrote:
> This is kind of gross.
> 
> It might be time to consider creating a real class hierarchy for Option:
> 
> class Option {}
> class SingleOption extends Option {}
> class MultipleOption extends Option {}
> class Flag extends Option {}
> class Separator extends Option {}
> 
> Then call this orderedOptions, or just not expose it publicly at all.

I thought about that, but Separators are really not options, both in the sense
of semantics and of utility. Almost anything people do with options wouldn't
make sense with separators. If we put separators in [options], every use of it
in ArgParser and Parser (or in users' external code) would have to add an
exception to ignore separators. That seems like a way bigger hassle than
dynamically typing this field.

https://codereview.chromium.org/1145413002/diff/1/lib/src/arg_parser.dart#new...
lib/src/arg_parser.dart:52: this.optionsAndSeparators = new
UnmodifiableListView(optionsAndSeparators),
On 2015/05/21 16:43:16, Bob Nystrom wrote:
> Long line.

Done.

https://codereview.chromium.org/1145413002/diff/1/lib/src/arg_parser.dart#new...
lib/src/arg_parser.dart:132: /// together visually.
On 2015/05/21 16:43:16, Bob Nystrom wrote:
> This paragraph is kind of choppy. How about just:
> 
> "In the usage text for the parser, this will appear between any options added
> before this call and ones added after it."

Done.

https://codereview.chromium.org/1145413002/diff/1/lib/src/usage.dart
File lib/src/usage.dart (right):

https://codereview.chromium.org/1145413002/diff/1/lib/src/usage.dart#newcode74
lib/src/usage.dart:74: var option = optionOrSeparator;
On 2015/05/21 16:43:16, Bob Nystrom wrote:
> Do you need an "as Option" here?

The analyzer infers it as dynamic, so it doesn't produce any warnings. That also
means it won't warn about misuse, though; would you prefer that I add it?

[Bob Nystrom, 2015-05-21 19:42:27]

https://codereview.chromium.org/1145413002/diff/1/lib/src/arg_parser.dart
File lib/src/arg_parser.dart (right):

https://codereview.chromium.org/1145413002/diff/1/lib/src/arg_parser.dart#new...
lib/src/arg_parser.dart:26: final List optionsAndSeparators;
On 2015/05/21 19:22:27, nweiz wrote:
> On 2015/05/21 16:43:16, Bob Nystrom wrote:
> > This is kind of gross.
> > 
> > It might be time to consider creating a real class hierarchy for Option:
> > 
> > class Option {}
> > class SingleOption extends Option {}
> > class MultipleOption extends Option {}
> > class Flag extends Option {}
> > class Separator extends Option {}
> > 
> > Then call this orderedOptions, or just not expose it publicly at all.
> 
> I thought about that, but Separators are really not options, both in the sense
> of semantics and of utility. Almost anything people do with options wouldn't
> make sense with separators.

True. It might be possible to come up with a name that represents the union of
options and separators, like "section" or something. <shrug>

> If we put separators in [options], every use of it
> in ArgParser and Parser (or in users' external code) would have to add an
> exception to ignore separators. That seems like a way bigger hassle than
> dynamically typing this field.

Oh, true. I wasn't thinking of actually putting the separators in the options
map.

How about just making optionalsAndSeparators private? It's really the name
that's most egregious.

https://codereview.chromium.org/1145413002/diff/1/lib/src/usage.dart
File lib/src/usage.dart (right):

https://codereview.chromium.org/1145413002/diff/1/lib/src/usage.dart#newcode74
lib/src/usage.dart:74: var option = optionOrSeparator;
On 2015/05/21 19:22:27, nweiz wrote:
> On 2015/05/21 16:43:16, Bob Nystrom wrote:
> > Do you need an "as Option" here?
> 
> The analyzer infers it as dynamic, so it doesn't produce any warnings. That
also
> means it won't warn about misuse, though; would you prefer that I add it?

Yeah, I think so.

[nweiz, 2015-05-21 19:52:31]

Code review changes

[nweiz, 2015-05-21 19:52:37]

https://codereview.chromium.org/1145413002/diff/1/lib/src/arg_parser.dart
File lib/src/arg_parser.dart (right):

https://codereview.chromium.org/1145413002/diff/1/lib/src/arg_parser.dart#new...
lib/src/arg_parser.dart:26: final List optionsAndSeparators;
On 2015/05/21 19:42:27, Bob Nystrom wrote:
> On 2015/05/21 19:22:27, nweiz wrote:
> > On 2015/05/21 16:43:16, Bob Nystrom wrote:
> > > This is kind of gross.
> > > 
> > > It might be time to consider creating a real class hierarchy for Option:
> > > 
> > > class Option {}
> > > class SingleOption extends Option {}
> > > class MultipleOption extends Option {}
> > > class Flag extends Option {}
> > > class Separator extends Option {}
> > > 
> > > Then call this orderedOptions, or just not expose it publicly at all.
> > 
> > I thought about that, but Separators are really not options, both in the
sense
> > of semantics and of utility. Almost anything people do with options wouldn't
> > make sense with separators.
> 
> True. It might be possible to come up with a name that represents the union of
> options and separators, like "section" or something. <shrug>
> 
> > If we put separators in [options], every use of it
> > in ArgParser and Parser (or in users' external code) would have to add an
> > exception to ignore separators. That seems like a way bigger hassle than
> > dynamically typing this field.
> 
> Oh, true. I wasn't thinking of actually putting the separators in the options
> map.
> 
> How about just making optionalsAndSeparators private? It's really the name
> that's most egregious.

Done.

https://codereview.chromium.org/1145413002/diff/1/lib/src/usage.dart
File lib/src/usage.dart (right):

https://codereview.chromium.org/1145413002/diff/1/lib/src/usage.dart#newcode74
lib/src/usage.dart:74: var option = optionOrSeparator;
On 2015/05/21 19:42:27, Bob Nystrom wrote:
> On 2015/05/21 19:22:27, nweiz wrote:
> > On 2015/05/21 16:43:16, Bob Nystrom wrote:
> > > Do you need an "as Option" here?
> > 
> > The analyzer infers it as dynamic, so it doesn't produce any warnings. That
> also
> > means it won't warn about misuse, though; would you prefer that I add it?
> 
> Yeah, I think so.

Done.

[Bob Nystrom, 2015-05-21 20:11:35]

LGTM! Don't forget to tag and publish.

[nweiz, 2015-05-21 20:29:54]

Committed patchset #3 (id:40001) manually as
8969a98dc94cda4aa4391499bebb697ecbffe04f (presubmit successful).