Add validation infrastructure for "pub lish".

Add validation infrastructure for "pub lish".

This also adds a simple validator.

BUG=7045
Committed: https://code.google.com/p/dart/source/detail?r=15765

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

I have some architecture feedback, but overall I really like this approach.

I don't know if now is the right time, but I think it would be useful to have
validators specify which operations they validate for. That would let us use the
same validator infrastructure to, for example, validate that a pubspec has a
name before you do pub install.

Thoughts?

https://codereview.chromium.org/11434118/diff/6001/utils/pub/validator.dart
File utils/pub/validator.dart (right):

https://codereview.chromium.org/11434118/diff/6001/utils/pub/validator.dart#n...
utils/pub/validator.dart:5: library validator;
"validation"?

https://codereview.chromium.org/11434118/diff/6001/utils/pub/validator.dart#n...
utils/pub/validator.dart:28: final errors = <String>[];
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?

https://codereview.chromium.org/11434118/diff/6001/utils/pub/validator.dart#n...
utils/pub/validator.dart:44: new PubspecFieldValidator(cache, entrypoint)
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?

https://codereview.chromium.org/11434118/diff/6001/utils/pub/validator.dart#n...
utils/pub/validator.dart:77: stdout.writeString("Package has ${warnings.length}
warning$s. Upload "
I think this should be moved out of the validation code and into the pub lish
command.

https://codereview.chromium.org/11434118/diff/6001/utils/pub/validator.dart#n...
utils/pub/validator.dart:81: throw "Package upload aborted.";
"aborted" -> "cancelled"

https://codereview.chromium.org/11434118/diff/6001/utils/tests/pub/test_pub.dart
File utils/tests/pub/test_pub.dart (right):

https://codereview.chromium.org/11434118/diff/6001/utils/tests/pub/test_pub.d...
utils/tests/pub/test_pub.dart:1366: return
timeout(_stdout.chain(consumeStringInputStream), 5000,
Make a constant for all of these "5000"s, please.

https://codereview.chromium.org/11434118/diff/6001/utils/tests/pub/validator_...
File utils/tests/pub/validator_test.dart (right):

https://codereview.chromium.org/11434118/diff/6001/utils/tests/pub/validator_...
utils/tests/pub/validator_test.dart:63: main() {
How about splitting these into separate unit tests and integration tests? Having
to go through the full upload process just to test each validator feels a bit
heavyweight (and slow). Maybe:

1. Unit tests for each validation rule that just use the validator API directly.
2. An integration test for upload that verifies that an upload aborts on some
arbitrarily chosen canary validation error.
3. Integration tests for aborting and continuing on a validation warning.

Thoughts?

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

I think we should wait on figuring out how this applies to installation.

https://codereview.chromium.org/11434118/diff/6001/utils/pub/validator.dart
File utils/pub/validator.dart (right):

https://codereview.chromium.org/11434118/diff/6001/utils/pub/validator.dart#n...
utils/pub/validator.dart:5: library validator;
On 2012/12/05 18:57:40, Bob Nystrom wrote:
> "validation"?

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

https://codereview.chromium.org/11434118/diff/6001/utils/pub/validator.dart#n...
utils/pub/validator.dart:28: final errors = <String>[];
On 2012/12/05 18:57:40, Bob Nystrom wrote:
> 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?

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.

https://codereview.chromium.org/11434118/diff/6001/utils/pub/validator.dart#n...
utils/pub/validator.dart:44: new PubspecFieldValidator(cache, entrypoint)
On 2012/12/05 18:57:40, Bob Nystrom wrote:
> 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?

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.

https://codereview.chromium.org/11434118/diff/6001/utils/pub/validator.dart#n...
utils/pub/validator.dart:77: stdout.writeString("Package has ${warnings.length}
warning$s. Upload "
On 2012/12/05 18:57:40, Bob Nystrom wrote:
> I think this should be moved out of the validation code and into the pub lish
> command.

Done.

https://codereview.chromium.org/11434118/diff/6001/utils/pub/validator.dart#n...
utils/pub/validator.dart:81: throw "Package upload aborted.";
On 2012/12/05 18:57:40, Bob Nystrom wrote:
> "aborted" -> "cancelled"

Done.

https://codereview.chromium.org/11434118/diff/6001/utils/tests/pub/test_pub.dart
File utils/tests/pub/test_pub.dart (right):

https://codereview.chromium.org/11434118/diff/6001/utils/tests/pub/test_pub.d...
utils/tests/pub/test_pub.dart:1366: return
timeout(_stdout.chain(consumeStringInputStream), 5000,
On 2012/12/05 18:57:40, Bob Nystrom wrote:
> Make a constant for all of these "5000"s, please.

Done.

https://codereview.chromium.org/11434118/diff/6001/utils/tests/pub/validator_...
File utils/tests/pub/validator_test.dart (right):

https://codereview.chromium.org/11434118/diff/6001/utils/tests/pub/validator_...
utils/tests/pub/validator_test.dart:63: main() {
On 2012/12/05 18:57:40, Bob Nystrom wrote:
> How about splitting these into separate unit tests and integration tests?
Having
> to go through the full upload process just to test each validator feels a bit
> heavyweight (and slow). Maybe:
> 
> 1. Unit tests for each validation rule that just use the validator API
directly.
> 2. An integration test for upload that verifies that an upload aborts on some
> arbitrarily chosen canary validation error.
> 3. Integration tests for aborting and continuing on a validation warning.
> 
> Thoughts?

Done.

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

LGTM.

https://codereview.chromium.org/11434118/diff/6001/utils/pub/validator.dart
File utils/pub/validator.dart (right):

https://codereview.chromium.org/11434118/diff/6001/utils/pub/validator.dart#n...
utils/pub/validator.dart:5: library validator;
On 2012/12/05 21:59:59, nweiz wrote:
> On 2012/12/05 18:57:40, Bob Nystrom wrote:
> > "validation"?
> 
> I think the library name should match the name of the class it defines.

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.

https://codereview.chromium.org/11434118/diff/6001/utils/pub/validator.dart#n...
utils/pub/validator.dart:28: final errors = <String>[];
On 2012/12/05 21:59:59, nweiz wrote:
> On 2012/12/05 18:57:40, Bob Nystrom wrote:
> > 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?
> 
> 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.

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.

https://codereview.chromium.org/11434118/diff/6001/utils/pub/validator.dart#n...
utils/pub/validator.dart:44: new PubspecFieldValidator(cache, entrypoint)
On 2012/12/05 21:59:59, nweiz wrote:
> On 2012/12/05 18:57:40, Bob Nystrom wrote:
> > 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?
> 
> 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.

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

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

https://codereview.chromium.org/11434118/diff/6001/utils/pub/validator.dart
File utils/pub/validator.dart (right):

https://codereview.chromium.org/11434118/diff/6001/utils/pub/validator.dart#n...
utils/pub/validator.dart:5: library validator;
On 2012/12/05 22:18:41, Bob Nystrom wrote:
> On 2012/12/05 21:59:59, nweiz wrote:
> > On 2012/12/05 18:57:40, Bob Nystrom wrote:
> > > "validation"?
> > 
> > I think the library name should match the name of the class it defines.
> 
> 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.

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.

https://codereview.chromium.org/11434118/diff/6001/utils/pub/validator.dart#n...
utils/pub/validator.dart:28: final errors = <String>[];
On 2012/12/05 22:18:41, Bob Nystrom wrote:
> On 2012/12/05 21:59:59, nweiz wrote:
> > On 2012/12/05 18:57:40, Bob Nystrom wrote:
> > > 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?
> > 
> > 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.
> 
> 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.

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.