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

pkg/args/lib/args.dart

Index: pkg/args/lib/args.dart
diff --git a/pkg/args/lib/args.dart b/pkg/args/lib/args.dart
index 6bdcb7e9805e1e54f32ac3fc2ae11d8c44e29e98..71fad7819b8eb47afe3202f7add87ed29ed6a30a 100644
--- a/pkg/args/lib/args.dart
+++ b/pkg/args/lib/args.dart
@@ -285,11 +285,6 @@ class ArgParser {
 
     // Make sure the abbreviation isn't too long or in use.
     if (abbr != null) {
-      if (abbr.length > 1) {
-        throw new ArgumentError(
-            'Abbreviation "$abbr" is longer than one character.');
-      }
-
       var existing = findByAbbreviation(abbr);
       if (existing != null) {
         throw new ArgumentError(
@@ -342,7 +337,7 @@ class ArgParser {
 class Option {
   final String name;
   final String abbreviation;
-  final List allowed;
+  final List<String> allowed;
   final defaultValue;
   final Function callback;
   final String help;
@@ -353,7 +348,41 @@ class Option {
 
   Option(this.name, this.abbreviation, this.help, this.allowed,
       this.allowedHelp, this.defaultValue, this.callback, {this.isFlag,
-      this.negatable, this.allowMultiple: false});
+      this.negatable, this.allowMultiple: false}) {
+    if(name == null) {

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

Space between keyword and "(" here and elsewhere ("if", "for", "while", etc.).

+      throw new ArgumentError('"name" cannot be null');

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

As dumb as it seems, almost all Dart code I've seen doesn't bother to explicitly
check for null arguments, so let's remove this.

+    } else if(name.isEmpty) {
+      throw new ArgumentError('"name" cannot be empty');

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

The quoting here is confusing. How about "Name cannot be empty.".

Can you make these error messages complete sentences too?

+    } else if(name.startsWith('-')) {
+      throw new ArgumentError('"name" cannot start with "-"');

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

Would be nice to show the offending argument: 'Name "$name" cannot start with
"-".'

+    }
+
+    // ensure name does not contain any invalid characters

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

Sentence capitalize and add a ".".

+    for(final char in _invalidChars) {

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

This seems like a good case for a regexp:

if (_invalidChars.hasMatch(name)) ...

where _invalidChars is:

final _invalidChars = new RegExp("[ \t\r\n\"'\\/]");

+      if(name.contains(char)) {
+        throw new ArgumentError('"name" contains invalid characters');
+      }
+      if(name.contains(char)) {
+        throw new ArgumentError('"name" contains invalid characters');
+      }
+    }
+
+    if(abbreviation != null) {
+      if(abbreviation.length != 1) {
+        throw new ArgumentError('"abbreviation" must be null or have length 1');
+      } else if(abbreviation == '-') {
+        throw new ArgumentError('"abbreviation" cannot be "-"');
+      }
+
+      for(final char in _invalidChars) {
+        if(abbreviation.contains(char)) {
+          throw new ArgumentError('"abbreviation" is an invalid valid');

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

This error message is a little odd. :)

+        }
+      }
+    }
+  }
+
+  const _invalidChars = const [' ', '\t', '\r', '\n', '"', '"', r'\', r'/'];

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

Since it isn't instance-specific and is private, how about we hoist this out of
the class?

[kevmoo-old, 2013/03/13 19:35:53]

My theory: since it's *only* used by the class, we keep the top-level namespace
clean and keep the definition close to the usage.

...I'm happy to defer to guidelines and convention, but I'd like to understand.

 }
 
 /**