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

pkg/args/lib/args.dart

 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 /**
  * This library lets you define parsers for parsing raw command-line arguments
  * into a set of options and values using [GNU][] and [POSIX][] style options.
  *
  * ## Defining options ##
  *
@@ skipping 267 matching lines @@
       Map<String, String> allowedHelp, defaultsTo,
       void callback(value), {bool isFlag, bool negatable: false,
       bool allowMultiple: false}) {
     // Make sure the name isn't in use.
     if (options.containsKey(name)) {
       throw new ArgumentError('Duplicate option "$name".');
     }
 
     // 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(
             'Abbreviation "$abbr" is already used by "${existing.name}".');
       }
     }
 
     options[name] = new Option(name, abbr, help, allowed, allowedHelp,
         defaultsTo, callback, isFlag: isFlag, negatable: negatable,
         allowMultiple: allowMultiple);
@@ skipping 32 matching lines @@
         orElse: () => null);
   }
 }
 
 /**
  * A command-line option. Includes both flags and options which take a value.
  */
 class Option {
   final String name;
   final String abbreviation;
-  final List allowed;
+  final List<String> allowed;
   final defaultValue;
   final Function callback;
   final String help;
   final Map<String, String> allowedHelp;
   final bool isFlag;
   final bool negatable;
   final bool allowMultiple;
 
   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.

 }
 
 /**
  * The results of parsing a series of command line arguments using
  * [ArgParser.parse()]. Includes the parsed options and any remaining unparsed
  * command line arguments.
  */
 class ArgResults {
   final Map _options;
 
@@ skipping 26 matching lines @@
           'Could not find an option named "$name".');
     }
 
     return _options[name];
   }
 
   /** Get the names of the options as a [Collection]. */
   Collection<String> get options => _options.keys.toList(growable: false);
 }