Harvest from pub. Mostly a straight copy, except:

Harvest from pub. Mostly a straight copy, except:

- Reorganized into smaller libraries.
- Added tests for prioritize() and antiprioritize().
- Filled in a few doc comments.

R=nweiz@google.com

Committed: c901a74427c0027a10ae67a182fb0c9886db3569

[Bob Nystrom, 2014-09-25 19:40:24]

rnystrom@google.com changed reviewers:
+ nweiz@google.com

[Bob Nystrom, 2014-09-25 19:40:25]

I'll pull the old stuff out of pub and bring this in to replace it in a separate
patch.

[nweiz, 2014-09-25 22:52:03]

https://chromiumcodereview.appspot.com/607663002/diff/1/README.md
File README.md (right):

https://chromiumcodereview.appspot.com/607663002/diff/1/README.md#newcode62
README.md:62: order which is different from strict comparison ordering. Any
stable version is considered higher priority than any unstable version. The
above versions, in priority order, are:
Long line.

https://chromiumcodereview.appspot.com/607663002/diff/1/README.md#newcode72
README.md:72: to it.
Nit: "in to" -> "into"

https://chromiumcodereview.appspot.com/607663002/diff/1/lib/pub_semver.dart
File lib/pub_semver.dart (right):

https://chromiumcodereview.appspot.com/607663002/diff/1/lib/pub_semver.dart#n...
lib/pub_semver.dart:10: /// [semver]: http://semver.org/
I'm not a fan of library docs when a README also exists. If you omit it, docgen
defaults to the README, which is usually more comprehensive.

https://chromiumcodereview.appspot.com/607663002/diff/1/lib/src/version_const...
File lib/src/version_constraint.dart (right):

https://chromiumcodereview.appspot.com/607663002/diff/1/lib/src/version_const...
lib/src/version_constraint.dart:21: /// A [VersionConstraint] that allows no
versions: i.e. the empty set.
I think we're supposed to avoid "i.e." in docs.

https://chromiumcodereview.appspot.com/607663002/diff/1/lib/src/version_const...
lib/src/version_constraint.dart:47: var constraints = <VersionConstraint>[];
Nit: no type annotations here or for the functions below.

It might be nice to use StringScanner here to get nice error messages.

https://chromiumcodereview.appspot.com/607663002/diff/1/lib/src/version_range...
File lib/src/version_range.dart (right):

https://chromiumcodereview.appspot.com/607663002/diff/1/lib/src/version_range...
lib/src/version_range.dart:19: final bool includeMax;
Document these, especially what it means for [min] and [max] to be null.

https://chromiumcodereview.appspot.com/607663002/diff/1/lib/src/version_range...
lib/src/version_range.dart:21: VersionRange({this.min, this.max,
Document this.

https://chromiumcodereview.appspot.com/607663002/diff/1/lib/src/version_range...
lib/src/version_range.dart:23: if (min != null && max != null && min > max) {
Nit: short-circuit to save a line.

https://chromiumcodereview.appspot.com/607663002/diff/1/lib/src/version_range...
lib/src/version_range.dart:42: /// Tests if [other] matches falls within this
version range.
-"matches"

https://chromiumcodereview.appspot.com/607663002/diff/1/pubspec.lock
File pubspec.lock (right):

https://chromiumcodereview.appspot.com/607663002/diff/1/pubspec.lock#newcode23
pubspec.lock:23: version: "0.11.0+5"
Don't check in the lockfile.

https://chromiumcodereview.appspot.com/607663002/diff/1/pubspec.yaml
File pubspec.yaml (right):

https://chromiumcodereview.appspot.com/607663002/diff/1/pubspec.yaml#newcode7
pubspec.yaml:7: homepage: http://github.com/dart-lang/pub_semver
How is this getting tested?

https://chromiumcodereview.appspot.com/607663002/diff/1/test/utils.dart
File test/utils.dart (right):

https://chromiumcodereview.appspot.com/607663002/diff/1/test/utils.dart#newco...
test/utils.dart:35: description.add(' ${_allow ? "allows" : "does not allow"}
versions');
Shouldn't this include _expected?

https://chromiumcodereview.appspot.com/607663002/diff/1/test/utils.dart#newco...
test/utils.dart:83: throwsArgError(function) {
This doesn't seem worth a helper function.

[Bob Nystrom, 2014-09-26 19:41:08]

https://chromiumcodereview.appspot.com/607663002/diff/1/README.md
File README.md (right):

https://chromiumcodereview.appspot.com/607663002/diff/1/README.md#newcode62
README.md:62: order which is different from strict comparison ordering. Any
stable version is considered higher priority than any unstable version. The
above versions, in priority order, are:
On 2014/09/25 22:52:02, nweiz wrote:
> Long line.

Done.

https://chromiumcodereview.appspot.com/607663002/diff/1/README.md#newcode72
README.md:72: to it.
On 2014/09/25 22:52:02, nweiz wrote:
> Nit: "in to" -> "into"

Done.

https://chromiumcodereview.appspot.com/607663002/diff/1/lib/pub_semver.dart
File lib/pub_semver.dart (right):

https://chromiumcodereview.appspot.com/607663002/diff/1/lib/pub_semver.dart#n...
lib/pub_semver.dart:10: /// [semver]: http://semver.org/
On 2014/09/25 22:52:02, nweiz wrote:
> I'm not a fan of library docs when a README also exists. If you omit it,
docgen
> defaults to the README, which is usually more comprehensive.

Done.

https://chromiumcodereview.appspot.com/607663002/diff/1/lib/src/version_const...
File lib/src/version_constraint.dart (right):

https://chromiumcodereview.appspot.com/607663002/diff/1/lib/src/version_const...
lib/src/version_constraint.dart:21: /// A [VersionConstraint] that allows no
versions: i.e. the empty set.
On 2014/09/25 22:52:02, nweiz wrote:
> I think we're supposed to avoid "i.e." in docs.

Done.

https://chromiumcodereview.appspot.com/607663002/diff/1/lib/src/version_const...
lib/src/version_constraint.dart:47: var constraints = <VersionConstraint>[];
On 2014/09/25 22:52:02, nweiz wrote:
> Nit: no type annotations here or for the functions below.

Done.

> It might be nice to use StringScanner here to get nice error messages.

I think that's probably overkill here. It already does an OK job of showing
where the error is.

https://chromiumcodereview.appspot.com/607663002/diff/1/lib/src/version_range...
File lib/src/version_range.dart (right):

https://chromiumcodereview.appspot.com/607663002/diff/1/lib/src/version_range...
lib/src/version_range.dart:19: final bool includeMax;
On 2014/09/25 22:52:02, nweiz wrote:
> Document these, especially what it means for [min] and [max] to be null.

Done.

https://chromiumcodereview.appspot.com/607663002/diff/1/lib/src/version_range...
lib/src/version_range.dart:21: VersionRange({this.min, this.max,
On 2014/09/25 22:52:02, nweiz wrote:
> Document this.

Done.

https://chromiumcodereview.appspot.com/607663002/diff/1/lib/src/version_range...
lib/src/version_range.dart:23: if (min != null && max != null && min > max) {
On 2014/09/25 22:52:02, nweiz wrote:
> Nit: short-circuit to save a line.

I would in most cases, but I think this is a bit easier to read this way, and
the extra line and indentation are minimal.

https://chromiumcodereview.appspot.com/607663002/diff/1/lib/src/version_range...
lib/src/version_range.dart:42: /// Tests if [other] matches falls within this
version range.
On 2014/09/25 22:52:02, nweiz wrote:
> -"matches"

Done.

https://chromiumcodereview.appspot.com/607663002/diff/1/pubspec.lock
File pubspec.lock (right):

https://chromiumcodereview.appspot.com/607663002/diff/1/pubspec.lock#newcode23
pubspec.lock:23: version: "0.11.0+5"
On 2014/09/25 22:52:02, nweiz wrote:
> Don't check in the lockfile.

Done.

https://chromiumcodereview.appspot.com/607663002/diff/1/pubspec.yaml
File pubspec.yaml (right):

https://chromiumcodereview.appspot.com/607663002/diff/1/pubspec.yaml#newcode7
pubspec.yaml:7: homepage: http://github.com/dart-lang/pub_semver
On 2014/09/25 22:52:02, nweiz wrote:
> How is this getting tested?

Right now, just manually.

I talked to Dan about it and, since this code is quite stable and implicitly
tested by pub, we're OK with it not being automatically tested until the new
bots are ready for it.

https://chromiumcodereview.appspot.com/607663002/diff/1/test/utils.dart
File test/utils.dart (right):

https://chromiumcodereview.appspot.com/607663002/diff/1/test/utils.dart#newco...
test/utils.dart:35: description.add(' ${_allow ? "allows" : "does not allow"}
versions');
On 2014/09/25 22:52:02, nweiz wrote:
> Shouldn't this include _expected?

describeMismatch() more or less covers this, but done.

https://chromiumcodereview.appspot.com/607663002/diff/1/test/utils.dart#newco...
test/utils.dart:83: throwsArgError(function) {
On 2014/09/25 22:52:02, nweiz wrote:
> This doesn't seem worth a helper function.

Done.

[nweiz, 2014-09-26 19:44:23]

lgtm

https://chromiumcodereview.appspot.com/607663002/diff/1/lib/src/version_const...
File lib/src/version_constraint.dart (right):

https://chromiumcodereview.appspot.com/607663002/diff/1/lib/src/version_const...
lib/src/version_constraint.dart:47: var constraints = <VersionConstraint>[];
On 2014/09/26 19:41:07, Bob Nystrom wrote:
> On 2014/09/25 22:52:02, nweiz wrote:
> > Nit: no type annotations here or for the functions below.
> 
> Done.
> 
> > It might be nice to use StringScanner here to get nice error messages.
> 
> I think that's probably overkill here. It already does an OK job of showing
> where the error is.

This array still has an annotation.

[Bob Nystrom, 2014-09-26 19:54:42]

Committed patchset #2 (id:20001) manually as c901a74 (presubmit successful).

[Bob Nystrom, 2014-11-10 22:14:06]

https://codereview.chromium.org/607663002/diff/1/lib/src/version_constraint.dart
File lib/src/version_constraint.dart (right):

https://codereview.chromium.org/607663002/diff/1/lib/src/version_constraint.d...
lib/src/version_constraint.dart:47: var constraints = <VersionConstraint>[];
On 2014/09/26 19:44:23, nweiz wrote:
> On 2014/09/26 19:41:07, Bob Nystrom wrote:
> > On 2014/09/25 22:52:02, nweiz wrote:
> > > Nit: no type annotations here or for the functions below.
> > 
> > Done.
> > 
> > > It might be nice to use StringScanner here to get nice error messages.
> > 
> > I think that's probably overkill here. It already does an OK job of showing
> > where the error is.
> 
> This array still has an annotation.

Done.