Add command support to args.

Add command support to args.

Committed: https://code.google.com/p/dart/source/detail?r=16988

[Bob Nystrom, 2013-01-10 22:32:13]

This isn't at the point yet where pub can use it, but it's a step in the right
direction. I wanted to get this first patch in so that I can move onto other
things for now.

[nweiz, 2013-01-11 00:06:06]

https://codereview.chromium.org/11819068/diff/5001/pkg/args/lib/args.dart
File pkg/args/lib/args.dart (right):

https://codereview.chromium.org/11819068/diff/5001/pkg/args/lib/args.dart#new...
pkg/args/lib/args.dart:245: */
When you have a sec, can you run your comment-fixing script on this code?

https://codereview.chromium.org/11819068/diff/5001/pkg/args/lib/args.dart#new...
pkg/args/lib/args.dart:356: class _ArgParser {
There are getting to be a lot of classes in this file. Maybe time to put stuff
in src?

Also it might accentuate the distinction between this and ArgParser if you just
called it _Parser.

https://codereview.chromium.org/11819068/diff/5001/pkg/args/lib/args.dart#new...
pkg/args/lib/args.dart:370: final ArgParser parser;
"grammar"? It's kind of confusing to read "parser" and have it not be an
instance of the class that you explicitly call out as the actual parser.

https://codereview.chromium.org/11819068/diff/5001/pkg/args/lib/args.dart#new...
pkg/args/lib/args.dart:438: // the stack gets them?
The deepest command should get the rest of the arguments.

https://codereview.chromium.org/11819068/diff/5001/pkg/args/lib/args.dart#new...
pkg/args/lib/args.dart:444: * current option is at [index]. Validates that there
is a valid value there.
"[index]" -> "[index])".

https://codereview.chromium.org/11819068/diff/5001/pkg/args/lib/args.dart#new...
pkg/args/lib/args.dart:455: 'Missing argument for "${option.name}".');
You should run these validations before you remove the argument from the list.
Also, shouldn't this produce a test error?

https://codereview.chromium.org/11819068/diff/5001/pkg/args/test/command_test...
File pkg/args/test/command_test.dart (right):

https://codereview.chromium.org/11819068/diff/5001/pkg/args/test/command_test...
pkg/args/test/command_test.dart:12: test('returns an ArgParser for the command',
() {
This seems kind of pedantic. I don't think it's worth testing this when you're
already testing all sorts of ArgParser behavior.

https://codereview.chromium.org/11819068/diff/5001/pkg/args/test/command_test...
pkg/args/test/command_test.dart:32: expect(args.command, isNotNull);
This seems redundant with the expect below.

https://codereview.chromium.org/11819068/diff/5001/pkg/args/test/command_test...
pkg/args/test/command_test.dart:130: test('does not parse a solo command before
the command', () {
"solo command" -> "solo command option"

[Bob Nystrom, 2013-01-11 17:59:49]

Thanks! Splitting it into sublibraries definitely made it easier to get around.

As a knock-on effect, more things are public now, but I think that actually
improves the API: the ArgParser and Options are more externally inspectable now.

https://codereview.chromium.org/11819068/diff/5001/pkg/args/lib/args.dart
File pkg/args/lib/args.dart (right):

https://codereview.chromium.org/11819068/diff/5001/pkg/args/lib/args.dart#new...
pkg/args/lib/args.dart:245: */
On 2013/01/11 00:06:06, nweiz wrote:
> When you have a sec, can you run your comment-fixing script on this code?

I thought about that, but I'd like to do that in a separate patch if it's OK.

https://codereview.chromium.org/11819068/diff/5001/pkg/args/lib/args.dart#new...
pkg/args/lib/args.dart:356: class _ArgParser {
On 2013/01/11 00:06:06, nweiz wrote:
> There are getting to be a lot of classes in this file. Maybe time to put stuff
> in src?

Great idea. Done.
 
> Also it might accentuate the distinction between this and ArgParser if you
just
> called it _Parser.

Also done. Well, just Parser since it's in a sublibrary.

https://codereview.chromium.org/11819068/diff/5001/pkg/args/lib/args.dart#new...
pkg/args/lib/args.dart:370: final ArgParser parser;
On 2013/01/11 00:06:06, nweiz wrote:
> "grammar"? It's kind of confusing to read "parser" and have it not be an
> instance of the class that you explicitly call out as the actual parser.

Done.

https://codereview.chromium.org/11819068/diff/5001/pkg/args/lib/args.dart#new...
pkg/args/lib/args.dart:438: // the stack gets them?
On 2013/01/11 00:06:06, nweiz wrote:
> The deepest command should get the rest of the arguments.

Done.

https://codereview.chromium.org/11819068/diff/5001/pkg/args/lib/args.dart#new...
pkg/args/lib/args.dart:444: * current option is at [index]. Validates that there
is a valid value there.
On 2013/01/11 00:06:06, nweiz wrote:
> "[index]" -> "[index])".

Fixed. That doc was out of date. The old parser didn't destructively consume the
arg list.

https://codereview.chromium.org/11819068/diff/5001/pkg/args/lib/args.dart#new...
pkg/args/lib/args.dart:455: 'Missing argument for "${option.name}".');
On 2013/01/11 00:06:06, nweiz wrote:
> You should run these validations before you remove the argument from the list.
> Also, shouldn't this produce a test error?

It's a little confusing. The first element in the list is the option. This is
reading the value after the option (the second element). So if your args are
"--mode debug", this consumes "--mode" and then reads the value.

This is because the calling code removes the option *after* calling this.
Except, after calling this, it ends up removing the option value instead, and
*this* function removes the option name.

But that's confusing, so I reorganized the code to be less crazy. Now the code
that calls this consumes the option first and then this consumes the value.

https://codereview.chromium.org/11819068/diff/5001/pkg/args/test/command_test...
File pkg/args/test/command_test.dart (right):

https://codereview.chromium.org/11819068/diff/5001/pkg/args/test/command_test...
pkg/args/test/command_test.dart:12: test('returns an ArgParser for the command',
() {
On 2013/01/11 00:06:06, nweiz wrote:
> This seems kind of pedantic. I don't think it's worth testing this when you're
> already testing all sorts of ArgParser behavior.

Removed. I'm never sure whether or not "nominality" is part of a library's
public API guarantees.

https://codereview.chromium.org/11819068/diff/5001/pkg/args/test/command_test...
pkg/args/test/command_test.dart:32: expect(args.command, isNotNull);
On 2013/01/11 00:06:06, nweiz wrote:
> This seems redundant with the expect below.

Removed. Made since at the time since this was the only test that passed at
first. :)

https://codereview.chromium.org/11819068/diff/5001/pkg/args/test/command_test...
pkg/args/test/command_test.dart:130: test('does not parse a solo command before
the command', () {
On 2013/01/11 00:06:06, nweiz wrote:
> "solo command" -> "solo command option"

Done.

[nweiz, 2013-01-11 21:43:09]

lgtm

https://codereview.chromium.org/11819068/diff/5001/pkg/args/lib/args.dart
File pkg/args/lib/args.dart (right):

https://codereview.chromium.org/11819068/diff/5001/pkg/args/lib/args.dart#new...
pkg/args/lib/args.dart:245: */
On 2013/01/11 17:59:49, Bob Nystrom wrote:
> On 2013/01/11 00:06:06, nweiz wrote:
> > When you have a sec, can you run your comment-fixing script on this code?
> 
> I thought about that, but I'd like to do that in a separate patch if it's OK.

sgtm