Added the ability to get the names of the options in a set of parsed arguments, and the default val…

Added the ability to get the names of the options in a set of parsed arguments, and the default value for an argument. These are useful so we can see what the user has explicitly set that is non-default behavior.
Committed: https://code.google.com/p/dart/source/detail?r=10904

[Bob Nystrom, 2012-08-16 21:26:58]

Tests?

https://chromiumcodereview.appspot.com/10828351/diff/1/lib/args/args.dart
File lib/args/args.dart (right):

https://chromiumcodereview.appspot.com/10828351/diff/1/lib/args/args.dart#new...
lib/args/args.dart:495: * if the user specified something other than the
default.
This method is useful in its own right, but if it's just for testing that, why
not have isDefault(option) on ArgResults?

https://chromiumcodereview.appspot.com/10828351/diff/1/lib/args/args.dart#new...
lib/args/args.dart:499: _options[option].defaultValue : null;
If the option name is invalid, this should throw an InvalidArgumentException,
not just silently return null.

https://chromiumcodereview.appspot.com/10828351/diff/1/lib/args/args.dart#new...
lib/args/args.dart:531: Collection<String> get options => _options.getKeys();
Instead of this, how about making ArgResults implement Collection itself?

[gram, 2012-08-16 21:55:53]

https://chromiumcodereview.appspot.com/10828351/diff/1/lib/args/args.dart
File lib/args/args.dart (right):

https://chromiumcodereview.appspot.com/10828351/diff/1/lib/args/args.dart#new...
lib/args/args.dart:495: * if the user specified something other than the
default.
On 2012/08/16 21:26:58, Bob Nystrom wrote:
> This method is useful in its own right, but if it's just for testing that, why
> not have isDefault(option) on ArgResults?

ArgResults does not have that information. We could add a flag but then there
are two choices, neither of which are great - have a second map and duplicate
all the keys, or change the map to contain classes with option data rather than
just values. This seemed like a lightweight approach that has a trivial
implementation and, as you point out, adds more flexibility than just the use
case that drove me to add it. Isn't solving a more general problem considered a
good thing, especially when it comes virtually for free? If we added
isDefault(), and then later on discovered we had other reasons for getting at
the default value, we might end up adding yet another method, and thus is born
needless complexity.

https://chromiumcodereview.appspot.com/10828351/diff/1/lib/args/args.dart#new...
lib/args/args.dart:499: _options[option].defaultValue : null;
On 2012/08/16 21:26:58, Bob Nystrom wrote:
> If the option name is invalid, this should throw an InvalidArgumentException,
> not just silently return null.

Done. I assume you mean IllegalArgument...

https://chromiumcodereview.appspot.com/10828351/diff/1/lib/args/args.dart#new...
lib/args/args.dart:531: Collection<String> get options => _options.getKeys();
On 2012/08/16 21:26:58, Bob Nystrom wrote:
> Instead of this, how about making ArgResults implement Collection itself?

That seems like a lot of extra effort. All I want is a way to get at the valid
keys for indexing. Would it be enough to  change the name to getKeys()? Or to
implement Iterable<String>? Or leave it as is?

[Bob Nystrom, 2012-08-16 22:22:09]

https://chromiumcodereview.appspot.com/10828351/diff/3002/lib/args/args.dart
File lib/args/args.dart (right):

https://chromiumcodereview.appspot.com/10828351/diff/3002/lib/args/args.dart#...
lib/args/args.dart:498: getDefault(String option) {
Delete blank line above this.

https://chromiumcodereview.appspot.com/10828351/diff/3002/lib/args/args.dart#...
lib/args/args.dart:502: throw new IllegalArgumentException('No option named
$option');
Nit, but when possible, I prefer validating the args at the top of the fn:

if (!_options.containsKey(option)) throw ...

https://chromiumcodereview.appspot.com/10828351/diff/3002/tests/lib/args/args...
File tests/lib/args/args_test.dart (right):

https://chromiumcodereview.appspot.com/10828351/diff/3002/tests/lib/args/args...
tests/lib/args/args_test.dart:385: test('queries the default value', () {
Move these to a separate group for the getDefault() method, please.

https://chromiumcodereview.appspot.com/10828351/diff/3002/tests/lib/args/args...
tests/lib/args/args_test.dart:387: parser.addOption('define', defaultsTo:'0',
allowMultiple: true);
Space after ":".

https://chromiumcodereview.appspot.com/10828351/diff/3002/tests/lib/args/args...
tests/lib/args/args_test.dart:389: expect(()=>parser.getDefault('undefine'),
If it's OK with you, I would make this a separate test:

test('throws if you pass an unknown option name', () { ... {)

https://chromiumcodereview.appspot.com/10828351/diff/3002/tests/lib/args/args...
tests/lib/args/args_test.dart:393: test('queries the set options', () {
This should be a separate group for the ".options" getter too.

[gram, 2012-08-16 23:20:17]

https://chromiumcodereview.appspot.com/10828351/diff/3002/lib/args/args.dart
File lib/args/args.dart (right):

https://chromiumcodereview.appspot.com/10828351/diff/3002/lib/args/args.dart#...
lib/args/args.dart:502: throw new IllegalArgumentException('No option named
$option');
On 2012/08/16 22:22:10, Bob Nystrom wrote:
> Nit, but when possible, I prefer validating the args at the top of the fn:
> 
> if (!_options.containsKey(option)) throw ...

Done.

https://chromiumcodereview.appspot.com/10828351/diff/3002/tests/lib/args/args...
File tests/lib/args/args_test.dart (right):

https://chromiumcodereview.appspot.com/10828351/diff/3002/tests/lib/args/args...
tests/lib/args/args_test.dart:385: test('queries the default value', () {
On 2012/08/16 22:22:10, Bob Nystrom wrote:
> Move these to a separate group for the getDefault() method, please.

Done.

https://chromiumcodereview.appspot.com/10828351/diff/3002/tests/lib/args/args...
tests/lib/args/args_test.dart:387: parser.addOption('define', defaultsTo:'0',
allowMultiple: true);
On 2012/08/16 22:22:10, Bob Nystrom wrote:
> Space after ":".

Done.

https://chromiumcodereview.appspot.com/10828351/diff/3002/tests/lib/args/args...
tests/lib/args/args_test.dart:389: expect(()=>parser.getDefault('undefine'),
On 2012/08/16 22:22:10, Bob Nystrom wrote:
> If it's OK with you, I would make this a separate test:
> 
> test('throws if you pass an unknown option name', () { ... {)

Done.

https://chromiumcodereview.appspot.com/10828351/diff/3002/tests/lib/args/args...
tests/lib/args/args_test.dart:393: test('queries the set options', () {
On 2012/08/16 22:22:10, Bob Nystrom wrote:
> This should be a separate group for the ".options" getter too.

Done.

[Bob Nystrom, 2012-08-17 16:07:35]

LGTM!