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) {
+      throw new ArgumentError('Name cannot be null.');

[kevmoo-old, 2013/03/13 19:46:17]

Seemed too weird to test for a Method Not Found error.

Aside from the fact that this is much more helpful for an end user.

Not trying to buck tradition...just nudge it a bit.

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

Yeah, don't test for that either.
I'm not crazy about the status quo here, but I think it's valuable to be
consistent and keep code simple. Most public APIs do not explicitly check for
null, and most tests of those APIs don't validate what happens when you pass
null.

In other words, almost everyone on the team acts as if types are not nullable,
even though they are.

+    } else if (name.isEmpty) {
+      throw new ArgumentError('Name cannot be empty.');
+    } else if (name.startsWith('-')) {
+      throw new ArgumentError('Name $name cannot start with "-".');
+    }
+
+    // Ensure name does not contain any invalid characters.
+    if (_invalidChars.hasMatch(name)) {
+      throw new ArgumentError('Name "$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 "-".');
+      }
+
+      if (_invalidChars.hasMatch(abbreviation)) {
+        throw new ArgumentError('Abbreviation is an invalid character.');
+      }
+    }
+  }
+
+  final _invalidChars = new RegExp(r'''[ \t\r\n"'\\/]''');

[kevmoo-old, 2013/03/13 19:46:17]

this is only used by the ctor of this class. I'm a bit fan of

1) keeping definitions close to their usage
2) minimizing top-level mess.

Again, not trying to buck tradition...just nudge it.

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

Fair point.
Top level stuff doesn't bother me if it's private. I like small libraries, so
the private top-level namespace is small and uncluttered.

Leaving it in the class is fine if you prefer it. If you do, it should be
static.

 }
 
 /**
  * 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);
 }