pkg/args Option should be more strict about names and abbreviations

pkg/args/test/args_test.dart

Index: pkg/args/test/args_test.dart
diff --git a/pkg/args/test/args_test.dart b/pkg/args/test/args_test.dart
index d807291e04d840e5e3f36f85463462deba04e1b9..4912c5d25bd63ce1da13c3d8e58507beb8a17d89 100644
--- a/pkg/args/test/args_test.dart
+++ b/pkg/args/test/args_test.dart
@@ -32,6 +32,24 @@ main() {
       var parser = new ArgParser();
       throwsIllegalArg(() => parser.addFlag('flummox', abbr: 'flu'));
     });
+
+    test('throws ArgumentError if a flag name is invalid', () {
+      var parser = new ArgParser();
+
+      for(final name in _invalidOptions) {

[Bob Nystrom, 2013/03/13 18:07:09]

To be consistent, we don't use final for local variables.

[kevmoo-old, 2013/03/13 19:30:48]

`final` is so awesome. I'd love someone to start a trend to the contrary.

...but I won't be that guy.

[Bob Nystrom, 2013/03/13 21:29:57]

If we had "val" I would be all about it. In the absence of that "var" is shorter
and more familiar. Given that we tend to keep methods pretty short I'm honestly
not convinced enforcing single-assignment for local variables really adds much.
(Fields are another matter of course.)

+        final reason = '${Error.safeToString(name)} is not valid';
+        throwsIllegalArg(() => parser.addFlag(name), reason: reason);
+      }
+    });
+
+    test('accepts valid flag names', () {
+      var parser = new ArgParser();
+
+      for(final name in _validOptions) {
+        final reason = '${Error.safeToString(name)} is valid';
+        expect(() => parser.addFlag(name), returnsNormally, reason: reason);
+      }
+    });
   });
 
   group('ArgParser.addOption()', () {
@@ -58,6 +76,46 @@ main() {
       var parser = new ArgParser();
       throwsIllegalArg(() => parser.addOption('flummox', abbr: 'flu'));
     });
+
+    test('throws ArgumentError if the abbreviation is empty', () {
+      var parser = new ArgParser();
+      throwsIllegalArg(() => parser.addOption('flummox', abbr: ''));
+    });
+
+    test('throws ArgumentError if the abbreviation is an invalid value', () {
+      var parser = new ArgParser();
+      for(final name in _invalidOptions.where((v) => v != null)) {
+        throwsIllegalArg(() => parser.addOption('flummox', abbr: name));
+      }
+    });
+
+    test('throws ArgumentError if the abbreviation is a dash', () {
+      var parser = new ArgParser();
+      throwsIllegalArg(() => parser.addOption('flummox', abbr: '-'));
+    });
+
+    test('allows explict null value for "abbr"', () {
+      var parser = new ArgParser();
+      expect(() => parser.addOption('flummox', abbr: null), returnsNormally);
+    });
+
+    test('throws ArgumentError if an option name is invalid', () {
+      var parser = new ArgParser();
+
+      for(final name in _invalidOptions) {
+        final reason = '${Error.safeToString(name)} is not valid';
+        throwsIllegalArg(() => parser.addOption(name), reason: reason);
+      }
+    });
+
+    test('accepts valid option names', () {
+      var parser = new ArgParser();
+
+      for(final name in _validOptions) {
+        final reason = '${Error.safeToString(name)} is valid';
+        expect(() => parser.addOption(name), returnsNormally, reason: reason);
+      }
+    });
   });
 
   group('ArgParser.getDefault()', () {
@@ -155,10 +213,43 @@ main() {
   });
 }
 
-throwsIllegalArg(function) {
-  expect(function, throwsArgumentError);
+throwsIllegalArg(function, {String reason: null}) {
+  expect(function, throwsArgumentError, reason: reason);
 }
 
 throwsFormat(ArgParser parser, List<String> args) {
   expect(() => parser.parse(args), throwsFormatException);
 }
+
+Iterable<String> get _invalidOptions {

[Bob Nystrom, 2013/03/13 18:07:09]

This may be a big gratuitous. :)

How about just a fixed list of strings with one containing each black-listed
character and some other characters? Also an empty string one too.

[kevmoo-old, 2013/03/13 19:30:48]

I take testing seriously.

[Bob Nystrom, 2013/03/13 21:29:57]

Testing is serious business. SGTM.

+  final options = [null, '', '-', '--', '--foo'];
+
+  const invalidChars = const [' ', '\t', '\r', '\n', '"', '"', r'\', r'/'];
+
+  for(final invalidChar in invalidChars) {
+    // solo
+    options.add(invalidChar);
+
+    // before
+    options.add('${invalidChar}foo');
+
+    // contains
+    options.add('foo${invalidChar}bar');
+
+    // ends with
+    options.add('foo${invalidChar}');
+  }
+
+  return options;
+}
+
+const _validOptions = const
+[
+ 'a' // one char
+ 'contains-dash',
+ 'contains_underscore',
+ 'ends-with-dash-',
+ 'contains--doubledash--',
+ '1starts-with-number',
+ 'contains-a-1number',
+ 'ends-with-a-number8'];