Add validation infrastructure for "pub lish".

utils/pub/validator.dart

Index: utils/pub/validator.dart
diff --git a/utils/pub/validator.dart b/utils/pub/validator.dart
new file mode 100644
index 0000000000000000000000000000000000000000..5431c69612fa045828f486d56f621e9a247475f6
--- /dev/null
+++ b/utils/pub/validator.dart
@@ -0,0 +1,85 @@
+// Copyright (c) 2012, 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.
+
+library validator;

[Bob Nystrom, 2012/12/05 18:57:40]

"validation"?

[nweiz, 2012/12/05 21:59:59]

I think the library name should match the name of the class it defines.

[Bob Nystrom, 2012/12/05 22:18:41]

I actually think it shouldn't since it may make the library name confusing if it
later evolves to have more than just that one class. Whenever possible, I prefer
inclusive, open-ended sounding library names, though that's not a strong rule.
(And it's one I violate if I intend the library to be imported with a prefix.)

I'd prefer "validation" here but if you're really attached to the current name,
I guess it works too.

[nweiz, 2012/12/06 00:06:10]

I think open-ended names are good for libraries that are going to contain a wide
variety of things -- io, utils, version_solver I suppose -- but this is intended
to be a simple single-class library, and I think it should be named the same as
all the other single-class libraries in pub.

+
+import 'dart:io';
+
+import 'entrypoint.dart';
+import 'io.dart';
+import 'system_cache.dart';
+import 'utils.dart';
+import 'validator/pubspec_field.dart';
+
+/// The base class for validators that check whether a package is fit for
+/// uploading. Each validator should override [errors], [warnings], or both to
+/// return lists of errors or warnings to display to the user. Errors will cause
+/// the package not to be uploaded; warnings will require the user to confirm
+/// the upload.
+abstract class Validator {
+  /// The user's system cache.
+  final SystemCache cache;
+
+  /// The entrypoint that's being validated.
+  final Entrypoint entrypoint;
+
+  /// The accumulated errors for this validator. Filled by calling [validate].
+  final errors = <String>[];

[Bob Nystrom, 2012/12/05 18:57:40]

Instead of exposing these directly to the subclasses, which then lets them do
other stuff with the list like remove, clear, etc., how about:

1. Make errors and warnings private.
2. Add error() and warning() methods to Validator that subclasses can call.
3. Make a private _validate() method that takes the collections and then calls
the public/protected validate(). This would also spare you from having to
flatten the resulting collections. Like:

abstract class Validator {
  List<String> _errors;
  List<String> _warnings;

  // Conceptually "protected".
  void error(String message) => _errors.add(message);
  void warning(String message) => _warnings.add(message);

  void _validate(List<String> errors, List<String> warnings) {
    _errors = errors;
    _warnings = warnings;
    validate();
  }

  void validate();

  // Sync for example purposes...
  static void runAll() {
    var errors = [];
    var warnings = [];
    for (var validator in validators) {
      validator._validate(errors, warnings);
    }
  }
}

WDYT?

[nweiz, 2012/12/05 21:59:59]

That seems like more machinery than it's worth to prevent us from doing things
we won't actually do.

Conceptually, I do think of "errors" and "warnings" as being streams, which your
API makes more explicit. I'd like to use the streams v2 API once it's available.
For now, though, I think sticking with the more straightforward API that doesn't
require extra parameter declarations is easier.

[Bob Nystrom, 2012/12/05 22:18:41]

This would only require one extra parameter declaration here in this library.
Subclasses of Validator see the same validate() signature. But it's not a big
deal either way.

[nweiz, 2012/12/06 00:06:10]

If we do it that way, it becomes impossible to use subclasses of validator
through any API other than Validator.runAll, which is less useful. See for
example my individual validator usage in validator_test.

+
+  /// The accumulated warnings for this validator. Filled by calling [validate].
+  final warnings = <String>[];
+
+  Validator(this.cache, this.entrypoint);
+
+  /// Validates the entrypoint, adding any errors and warnings to [errors] and
+  /// [warnings], respectively.
+  Future validate();
+
+  /// Run all validators on the [entrypoint] package. The future will complete
+  /// successfully if validation succeeded, and complete with an error if it
+  /// failed.
+  static Future runAll(SystemCache cache, Entrypoint entrypoint) {
+    var validators = [
+      new PubspecFieldValidator(cache, entrypoint)

[Bob Nystrom, 2012/12/05 18:57:40]

What's the granularity that you have in mind for a validator? Is there a reason
to do both authors and homepage in one validator?

[nweiz, 2012/12/05 21:59:59]

It's a bit wishy-washy; basically I wanted to strike a balance somewhere between
having lots of validators that each do almost nothing and having one validator
that does everything.

Roughly speaking, each validator looks at one piece of information. Both fields
are part of the pubspec, so they're in the same validator. If multiple
validations need to share some logic, I'll probably also put them in the same
validator. But I'm mostly playing it by ear.

[Bob Nystrom, 2012/12/05 22:18:41]

OK. Once we have more validation in place, we can rebalance things if we want.

+    ];
+
+    // TODO(nweiz): The sleep 0 here forces us to go async. This works around
+    // 3356, which causes a bug if all validators are (synchronously) using
+    // Future.immediate and an error is thrown before a handler is set up.
+    return sleep(0).chain((_) {
+      return Futures.wait(validators.map((validator) => validator.validate()));
+    }).chain((_) {
+      var errors = flatten(validators.map((validator) => validator.errors));
+      var warnings = flatten(validators.map((validator) => validator.warnings));
+
+      if (errors.isEmpty && warnings.isEmpty) return new Future.immediate(null);
+
+      if (!errors.isEmpty) {
+        printError("== Errors:");
+        for (var error in errors) {
+          printError("* $error");
+        }
+        printError("");
+      }
+
+      if (!warnings.isEmpty) {
+        printError("== Warnings:");
+        for (var warning in warnings) {
+          printError("* $warning");
+        }
+        printError("");
+      }
+
+      if (!errors.isEmpty) throw "Package validation failed.";
+
+      var s = warnings.length == 1 ? '' : 's';
+      stdout.writeString("Package has ${warnings.length} warning$s. Upload "

[Bob Nystrom, 2012/12/05 18:57:40]

I think this should be moved out of the validation code and into the pub lish
command.

[nweiz, 2012/12/05 21:59:59]

Done.

+          "anyway (y/n)? ");
+      return readLine().transform((line) {
+        if (new RegExp(r"^[yY]").hasMatch(line)) return;
+        throw "Package upload aborted.";

[Bob Nystrom, 2012/12/05 18:57:40]

"aborted" -> "cancelled"

[nweiz, 2012/12/05 21:59:59]

Done.

+      });
+    });
+  }
+}