Ignore unhandled arguments instead of stopping parsing.

Ignore unhandled arguments instead of stopping parsing.

BUG=5262

[Bob Nystrom, 2013-01-09 22:05:36]

Peter, I think this gets you a bit closer to being able to use this in dart2js.
One feature that's probably still missing is handling comma-separated multiple
values. Currently, args *does* support multi-valued options, but it can't parse
them like:

  $ dart2js -mdebug,release

You'd have to do:

  $ dart2js -m debug -m release

If you'd like, feel free to file a bug for that.

[Emily Fortuna, 2013-01-09 23:29:18]

lgtm

[Bob Nystrom, 2013-01-10 00:55:43]

On 2013/01/09 23:29:18, Emily Fortuna wrote:
> lgtm

Nathan pointed out to me that this will, alas, break pub. I'll see if I can get
proper command support working in the args library to fix that. Until then, this
patch will have to sit a while.

[ahe, 2013-01-10 07:39:26]

I'm looking forward to getting rid of the current arg parsing hack in dart2js. I
don't understand how dart2js can use libraries from pkg as these aren't
(supposed) to be in the SDK.

[Bob Nystrom, 2013-01-10 17:29:47]

On 2013/01/10 07:39:26, ahe wrote:
> I'm looking forward to getting rid of the current arg parsing hack in dart2js.
I
> don't understand how dart2js can use libraries from pkg as these aren't
> (supposed) to be in the SDK.

The packages under pkg are currently in the SDK and are "publicly" in the SDK in
that other packages can depend on them using "SDK dependencies".

At some point in the future, "SDK dependencies" will go away. To depend on a
package, you'll have to depend on it from pub.dartlang.org (or git, or
whatever).

When that happens, pub will still need some of those packages to be physically
inside the SDK because it uses them itself internally. To make that work, our
plan is that the packages will still exist inside the SDK, but they'll be
stuffed under lib/_internal or something like that. Pub will be able to find
them, but their existence will be an implementation detail. From the user's
perspective they are not "in the SDK".

In other words, we will vendor them in like we do other third party
dependencies.

[ahe, 2013-01-10 19:51:40]

Test LGTM, didn't look at implementation.

https://codereview.chromium.org/11824044/diff/1/pkg/args/test/parse_test.dart
File pkg/args/test/parse_test.dart (right):

https://codereview.chromium.org/11824044/diff/1/pkg/args/test/parse_test.dart...
pkg/args/test/parse_test.dart:397: expect(results.rest, orderedEquals(['a', 'b',
'd']));
Could you add a test that you reject unknown arguments that look like options?

[Bob Nystrom, 2013-01-10 23:16:40]

https://codereview.chromium.org/11824044/diff/1/pkg/args/test/parse_test.dart
File pkg/args/test/parse_test.dart (right):

https://codereview.chromium.org/11824044/diff/1/pkg/args/test/parse_test.dart...
pkg/args/test/parse_test.dart:397: expect(results.rest, orderedEquals(['a', 'b',
'd']));
On 2013/01/10 19:51:40, ahe wrote:
> Could you add a test that you reject unknown arguments that look like options?

The tests on 252 and 329 should cover that, I think.

[ahe, 2013-01-11 05:37:28]

Tests still LGTM.

Have another feature request, but it isn't a show stopper for dart2js.

https://codereview.chromium.org/11824044/diff/1/pkg/args/test/parse_test.dart
File pkg/args/test/parse_test.dart (right):

https://codereview.chromium.org/11824044/diff/1/pkg/args/test/parse_test.dart...
pkg/args/test/parse_test.dart:397: expect(results.rest, orderedEquals(['a', 'b',
'd']));
On 2013/01/10 23:16:40, Bob Nystrom wrote:
> On 2013/01/10 19:51:40, ahe wrote:
> > Could you add a test that you reject unknown arguments that look like
options?
> 
> The tests on 252 and 329 should cover that, I think.

Yes. I guess I was skipping exceptions when I scanned for this.

Would it be possible to provide an onError callback so you can handle these
situations without throwing an exception?

I have two use cases in mind:

* You wrap another application, but want to add a few additional arguments. So
the facade will pass everything through that it doesn't understand.

* You can easily provide a "showUsageAndExit" method.

[Bob Nystrom, 2013-01-11 17:03:21]

On 2013/01/11 05:37:28, ahe wrote:
> Tests still LGTM.
> 
> Have another feature request, but it isn't a show stopper for dart2js.
> 
> https://codereview.chromium.org/11824044/diff/1/pkg/args/test/parse_test.dart
> File pkg/args/test/parse_test.dart (right):
> 
>
https://codereview.chromium.org/11824044/diff/1/pkg/args/test/parse_test.dart...
> pkg/args/test/parse_test.dart:397: expect(results.rest, orderedEquals(['a',
'b',
> 'd']));
> On 2013/01/10 23:16:40, Bob Nystrom wrote:
> > On 2013/01/10 19:51:40, ahe wrote:
> > > Could you add a test that you reject unknown arguments that look like
> options?
> > 
> > The tests on 252 and 329 should cover that, I think.
> 
> Yes. I guess I was skipping exceptions when I scanned for this.
> 
> Would it be possible to provide an onError callback so you can handle these
> situations without throwing an exception?

I'm not generally a fan of errbacks for error-handling in synchronous code, but
I would like a nice answer for your use case.

> 
> I have two use cases in mind:
> 
> * You wrap another application, but want to add a few additional arguments. So
> the facade will pass everything through that it doesn't understand.

This patch makes it so that unhandled arguments pass through, but unhandled
arguments that look like options don't. I was thinking of adding an optional
parameter to parse(): parse(ignoreUnknownOptions: true);

If you pass that, then unhandled options will be treated like unhandled
arguments and end up in the ArgResults.rest. Would that work for this?

> 
> * You can easily provide a "showUsageAndExit" method.

Yeah, I'd definely like a method that does all of the basic stuff in one step:

1. Parse the args directly from new Options().arguments.
2. If "--help" was passed, show the help and exit.
3. If the parse failed, show the help and exit.
4. Otherwise, return the results.

The main sticking points are:

1. Pub wouldn't actually use this functionality until command support is in
args. It needs to have its own usage text to show those right now.

2. Currently, args isn't actually coupled to dart:io. I'd be surprised if it was
ever used in an environment where dart:io isn't available, but I tried to avoid
that coupling, which is why you always have to pass in the command-line
arguments right now.

I'd be OK with giving up #2, but I'd like to fix #1 first. I find I do better
API design if I have a real-world app to dogfood it against.

If you'd like, can you file bugs for these? Otherwise, I will definitely forget.
Thanks!