Allow whitespace in version constraints.

utils/pub/version.dart

Index: utils/pub/version.dart
diff --git a/utils/pub/version.dart b/utils/pub/version.dart
index 840dd4c164182b1087aa5c9d8921cf7072818735..3e9b7f6e9708033a260eac7d7c5b959f817a1a40 100644
--- a/utils/pub/version.dart
+++ b/utils/pub/version.dart
@@ -11,18 +11,25 @@ import 'dart:math';
 
 import 'utils.dart';
 
+
+/// Regex that matches a version number at the beginning of a string.
+final _START_VERSION = new RegExp(
+    r'^'                                        // Start at beginning.
+    r'(\d+).(\d+).(\d+)'                        // Version number.
+    r'(-([0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*))?'    // Pre-release.
+    r'(\+([0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*))?'); // Build.
+
+/// Like [_START_VERSION] but matches the entire string.
+final _COMPLETE_VERSION = new RegExp("${_START_VERSION.pattern}\$");
+
+/// Parses a comparison operator ("<", ">", "<=", or ">=") at the beginning of
+/// a string.
+final _START_COMPARISON = new RegExp(r"[<>]=?");

[nweiz, 2013/02/21 01:31:11]

This should start with "^".

[Bob Nystrom, 2013/02/21 20:00:50]

Oops. Done.

+
 /// A parsed semantic version number.
 class Version implements Comparable<Version>, VersionConstraint {
   /// No released version: i.e. "0.0.0".
   static Version get none => new Version(0, 0, 0);
-
-  static final _PARSE_REGEX = new RegExp(
-      r'^'                                       // Start at beginning.
-      r'(\d+).(\d+).(\d+)'                       // Version number.
-      r'(-([0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*))?'   // Pre-release.
-      r'(\+([0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*))?'  // Build.
-      r'$');                                     // Consume entire string.
-
   /// The major version number: "1" in "1.2.3".
   final int major;
 
@@ -51,7 +58,7 @@ class Version implements Comparable<Version>, VersionConstraint {
 
   /// Creates a new [Version] by parsing [text].
   factory Version.parse(String text) {
-    final match = _PARSE_REGEX.firstMatch(text);
+    final match = _COMPLETE_VERSION.firstMatch(text);
     if (match == null) {
       throw new FormatException('Could not parse "$text".');
     }
@@ -214,28 +221,104 @@ abstract class VersionConstraint {
   /// A [VersionConstraint] that allows no versions: i.e. the empty set.
   static VersionConstraint empty = const _EmptyVersion();
 
-  /// Parses a version constraint. This string is a space-separated series of
-  /// version parts. Each part can be one of:
+  /// Parses a version constraint. This string is a series of version parts.
+  /// Each part can be one of:
   ///
   ///   * A version string like `1.2.3`. In other words, anything that can be
   ///     parsed by [Version.parse()].
   ///   * A comparison operator (`<`, `>`, `<=`, or `>=`) followed by a version
-  ///     string. There cannot be a space between the operator and the version.
+  ///     string.
+  ///
+  /// Whitespace is ignored.
   ///
   /// Examples:
   ///
   ///     1.2.3-alpha
   ///     <=5.1.4
-  ///     >2.0.4 <=2.4.6
+  ///     >2.0.4 <= 2.4.6
+  ///     any
   factory VersionConstraint.parse(String text) {
-    if (text.trim() == '') {
-      throw new FormatException('Cannot parse an empty string.');
+    var originalText = text;
+    var constraints = <VersionConstraint>[];
+
+    void skipWhitespace() {
+      text = text.trim();
     }
 
-    // Split it into space-separated parts.
-    var constraints = <VersionConstraint>[];
-    for (var part in text.split(' ')) {
-      constraints.add(_parseSingleConstraint(part));
+    // Try to parse and consume "any".
+    VersionRange matchAny() {
+      // TODO(rnystrom): This doesn't require whitespace or a punctuator to
+      // separate "any". So this is valid: "anyany>1.0.0any". Is that OK?

[nweiz, 2013/02/21 01:31:11]

It's weird that we allow "any" to be alongside any other version constraints,
but even given that "anyany>1.0.0any" is awful and shouldn't be allowed.

[Bob Nystrom, 2013/02/21 20:00:50]

You're exactly right. Thinking of "any" as a constraint *part* is the error
here. It should be a standalone constraint.
Agreed. Fixed. Now the rule is a constraint is either "any" or a series of
parts.

Much cleaner now.

+      if (!text.startsWith("any")) return null;
+      text = text.substring("any".length);
+      return new VersionRange();
+    }
+
+    // Try to parse and consume a version number.
+    Version matchVersion() {
+      var version = _START_VERSION.firstMatch(text);
+      if (version == null) return null;
+
+      text = text.substring(version.end);
+      return new Version.parse(version[0]);
+    }
+
+    // Try to parse and consume a comparison operator followed by a version.
+    VersionConstraint matchComparison() {
+      var comparison = _START_COMPARISON.firstMatch(text);
+      if (comparison == null) return null;
+
+      var op = comparison[0];
+      text = text.substring(comparison.end);
+      skipWhitespace();
+
+      var version = matchVersion();
+      if (version == null) {
+        throw new FormatException('Expected version number after "$op" in '
+            '"$originalText", got "$text".');

[nweiz, 2013/02/21 01:31:11]

I still wish these exceptions gave more context, although that's more of an
issue with the yaml library than this code.

[Bob Nystrom, 2013/02/21 20:00:50]

There's not much Version can do here. What we can (and probably should) do is
catch this exception in the pubspec parser and wrap it in that additional
context. But that's a separate change.

+      }
+
+      switch (op) {
+        case '<=':
+          return new VersionRange(max: version, includeMax: true);
+        case '<':
+          return new VersionRange(max: version, includeMax: false);
+        case '>=':
+          return new VersionRange(min: version, includeMin: true);
+        case '>':
+          return new VersionRange(min: version, includeMin: false);
+      }
+    }
+
+    while (true) {
+      skipWhitespace();
+      if (text.isEmpty) break;
+
+      var any = matchAny();
+      if (any != null) {
+        constraints.add(any);
+        continue;
+      }
+
+      var version = matchVersion();
+      if (version != null) {
+        constraints.add(version);
+        continue;
+      }
+
+      var comparison = matchComparison();
+      if (comparison != null) {
+        constraints.add(comparison);
+        continue;
+      }
+
+      // If we got here, we couldn't parse the remaining string.
+      throw new FormatException('Could not parse version "$originalText". '
+          'Unknown text at "$text".');
+    }
+
+    if (constraints.isEmpty) {
+      throw new FormatException('Cannot parse an empty string.');
     }
 
     return new VersionConstraint.intersection(constraints);
@@ -266,32 +349,6 @@ abstract class VersionConstraint {
   /// Creates a new [VersionConstraint] that only allows [Version]s allowed by
   /// both this and [other].
   VersionConstraint intersect(VersionConstraint other);
-
-  static VersionConstraint _parseSingleConstraint(String text) {
-    if (text == 'any') {
-      return new VersionRange();
-    }
-
-    // TODO(rnystrom): Consider other syntaxes for version constraints. This
-    // one is whitespace sensitive (you can't do "< 1.2.3") and "<" is
-    // unfortunately meaningful in YAML, requiring it to be quoted in a
-    // pubspec.
-    // See if it's a comparison operator followed by a version, like ">1.2.3".
-    var match = new RegExp(r"^([<>]=?)?(.*)$").firstMatch(text);
-    if (match != null) {
-      var comparison = match[1];
-      var version = new Version.parse(match[2]);
-      switch (match[1]) {
-        case '<=': return new VersionRange(max: version, includeMax: true);
-        case '<': return new VersionRange(max: version, includeMax: false);
-        case '>=': return new VersionRange(min: version, includeMin: true);
-        case '>': return new VersionRange(min: version, includeMin: false);
-      }
-    }
-
-    // Otherwise, it must be an explicit version.
-    return new Version.parse(text);
-  }
 }
 
 /// Constrains versions to a fall within a given range. If there is a minimum,