Add more set-like version constraint operations.

Add more set-like version constraint operations.

This adds support for union(), as well as checking if constraints are supersets
of or disjoint with one another. These are necessary for some version solver
work I'm doing in pub.

R=rnystrom@google.com

Committed: https://github.com/dart-lang/pub_semver/commit/70bb15f1004b40fff934286c829cc5b54aa04e86

[nweiz, 2015-05-05 19:01:22]

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

[Bob Nystrom, 2015-05-05 20:49:09]

https://codereview.chromium.org/1127783002/diff/20001/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/1127783002/diff/20001/lib/src/utils.dart#newc...
lib/src/utils.dart:39: 
Instead of these functions, how about just making Version implement
VersionRange?

https://codereview.chromium.org/1127783002/diff/20001/lib/src/utils.dart#newc...
lib/src/utils.dart:47: bool _includeMin(VersionConstraint constraint) =>
How about "_includesMin"?

https://codereview.chromium.org/1127783002/diff/20001/lib/src/utils.dart#newc...
lib/src/utils.dart:57: bool _includeMax(VersionConstraint constraint) =>
Ditto.

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version.dart
File lib/src/version.dart (right):

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version.dart#ne...
lib/src/version.dart:240: bool allowsAll(VersionConstraint other) =>
other.isEmpty || other == this;
What about:

var v123 = new Version.parse("1.2.3");
v123.allowsAll(new VersionRange(min: v123, max: v123, includeMin: true,
includeMax: true);

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_range.dart
File lib/src/version_range.dart (right):

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_range.d...
lib/src/version_range.dart:105: if (other is Version) return allows(other);
All these type checks make me think it might be cleaner to use the visitor
pattern:

bool allowsAll(VersionConstraint other) => other.versionRangeAllowsAll(this);
bool allowsAny(VersionConstraint other) => other.versionRangeAllowsAny(this);

That would let you define specific methods for each pair of types. Thoughts?

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_range.d...
lib/src/version_range.dart:140: if (other.min == null) return true;
How about hoisting this out of the outer loop? It's a little hard to grok this
as it is. More like:

if (min == null && other.min == null) return true;
if (min == null || min < other.min) { ... }

Some comments would help too.

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_range.d...
lib/src/version_range.dart:242: var edgesTouch = max == other.min && (includeMax
|| other.includeMin);
What about the other order:

other.max == min && (other.includeMax || includeMin);

?

Can you just use allowsAny(other) here?

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_range.d...
lib/src/version_range.dart:254: } else if (other.min == null || other.min <
unionMin) {
I think it's a bit easier to read if you use "min" instead of "unionMin" here...

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_range.d...
lib/src/version_range.dart:257: } else if (unionMin == other.min &&
other.includeMin) {
...and here. That way the reader doesn't have to see if the assignments to
unionMin affect it.

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_union.dart
File lib/src/version_union.dart (right):

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_union.d...
lib/src/version_union.dart:43: constraints = constraints.expand((constraint) {
Instead of reassigning, how about:

var flattened = ...

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_union.d...
lib/src/version_union.dart:66: for (var constraint in constraints) {
// If this constraint doesn't touch the previous one.

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_union.d...
lib/src/version_union.dart:81: : constraints = new
UnmodifiableListView(constraints);
If this is a package private class, is this worth doing?

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_union.d...
lib/src/version_union.dart:94: while (ourConstraints.current != null &&
theirConstraints.current != null) {
Checking for .current == null feels sketchy to me. I know null should never be a
value in the list, but it still feels like the return value of .moveNext() is a
clearer indication of end-of-sequence to me.

Either way, this is a pretty neat algorithm. :)

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_union.d...
lib/src/version_union.dart:149: // constraints have a change to match multiple
small constraints that they
"change" -> "chance".

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_union.d...
lib/src/version_union.dart:167: List<VersionConstraint>
_constraintsFor(VersionConstraint constraint) {
Make this a function?

https://codereview.chromium.org/1127783002/diff/20001/test/version_range_test...
File test/version_range_test.dart (right):

https://codereview.chromium.org/1127783002/diff/20001/test/version_range_test...
test/version_range_test.dart:176: expect(range.allowsAll(range), isTrue);
Maybe test against new VersionRange() (i.e. the open range).

https://codereview.chromium.org/1127783002/diff/20001/test/version_range_test...
test/version_range_test.dart:373: expect(result, allows(v123));
To match the next tests, maybe put a doesNotAllow here for the gap between the
two?

https://codereview.chromium.org/1127783002/diff/20001/test/version_union_test...
File test/version_union_test.dart (right):

https://codereview.chromium.org/1127783002/diff/20001/test/version_union_test...
test/version_union_test.dart:12: group('factory', () {
Test an empty list.

[nweiz, 2015-05-05 22:52:27]

Code review changes

[nweiz, 2015-05-05 22:52:34]

https://codereview.chromium.org/1127783002/diff/20001/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/1127783002/diff/20001/lib/src/utils.dart#newc...
lib/src/utils.dart:39: 
On 2015/05/05 20:49:09, Bob Nystrom wrote:
> Instead of these functions, how about just making Version implement
> VersionRange?

Done.

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version.dart
File lib/src/version.dart (right):

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version.dart#ne...
lib/src/version.dart:240: bool allowsAll(VersionConstraint other) =>
other.isEmpty || other == this;
On 2015/05/05 20:49:09, Bob Nystrom wrote:
> What about:
> 
> var v123 = new Version.parse("1.2.3");
> v123.allowsAll(new VersionRange(min: v123, max: v123, includeMin: true,
> includeMax: true);

In general, we don't handle VersionRanges that look like Versions—otherwise they
would be "=="-equivalent. If we do handle them, we should make sure they're
equivalent in every way, but I don't see an especially clean way of doing so.
Since you can't naturally end up with such a version range, I'm not too worried
about it.

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_range.dart
File lib/src/version_range.dart (right):

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_range.d...
lib/src/version_range.dart:105: if (other is Version) return allows(other);
On 2015/05/05 20:49:09, Bob Nystrom wrote:
> All these type checks make me think it might be cleaner to use the visitor
> pattern:
> 
> bool allowsAll(VersionConstraint other) => other.versionRangeAllowsAll(this);
> bool allowsAny(VersionConstraint other) => other.versionRangeAllowsAny(this);
> 
> That would let you define specific methods for each pair of types. Thoughts?

That seems like a lot more conceptual overhead, and I don't think it's useful
for users to define those methods on VersionConstraint.

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_range.d...
lib/src/version_range.dart:140: if (other.min == null) return true;
On 2015/05/05 20:49:09, Bob Nystrom wrote:
> How about hoisting this out of the outer loop? It's a little hard to grok this
> as it is. More like:
> 
> if (min == null && other.min == null) return true;
> if (min == null || min < other.min) { ... }
> 
> Some comments would help too.

Done.

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_range.d...
lib/src/version_range.dart:242: var edgesTouch = max == other.min && (includeMax
|| other.includeMin);
On 2015/05/05 20:49:09, Bob Nystrom wrote:
> What about the other order:
> 
> other.max == min && (other.includeMax || includeMin);
> 
> ?

Done.

> Can you just use allowsAny(other) here?

Done.

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_range.d...
lib/src/version_range.dart:254: } else if (other.min == null || other.min <
unionMin) {
On 2015/05/05 20:49:09, Bob Nystrom wrote:
> I think it's a bit easier to read if you use "min" instead of "unionMin"
here...

I was mirroring intersect(), but sure.

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_range.d...
lib/src/version_range.dart:257: } else if (unionMin == other.min &&
other.includeMin) {
On 2015/05/05 20:49:09, Bob Nystrom wrote:
> ...and here. That way the reader doesn't have to see if the assignments to
> unionMin affect it.

Done.

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_union.dart
File lib/src/version_union.dart (right):

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_union.d...
lib/src/version_union.dart:43: constraints = constraints.expand((constraint) {
On 2015/05/05 20:49:09, Bob Nystrom wrote:
> Instead of reassigning, how about:
> 
> var flattened = ...

Done.

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_union.d...
lib/src/version_union.dart:66: for (var constraint in constraints) {
On 2015/05/05 20:49:09, Bob Nystrom wrote:
> // If this constraint doesn't touch the previous one.

Done.

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_union.d...
lib/src/version_union.dart:81: : constraints = new
UnmodifiableListView(constraints);
On 2015/05/05 20:49:09, Bob Nystrom wrote:
> If this is a package private class, is this worth doing?

Probably not. Removed.

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_union.d...
lib/src/version_union.dart:94: while (ourConstraints.current != null &&
theirConstraints.current != null) {
On 2015/05/05 20:49:09, Bob Nystrom wrote:
> Checking for .current == null feels sketchy to me. I know null should never be
a
> value in the list, but it still feels like the return value of .moveNext() is
a
> clearer indication of end-of-sequence to me.
> 
> Either way, this is a pretty neat algorithm. :)

I don't think there's a clean way to do that. Even if we do "while (true)" and
break in the body, it handles the original calls to moveNext() much less
gracefully.

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_union.d...
lib/src/version_union.dart:149: // constraints have a change to match multiple
small constraints that they
On 2015/05/05 20:49:09, Bob Nystrom wrote:
> "change" -> "chance".

Done.

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_union.d...
lib/src/version_union.dart:167: List<VersionConstraint>
_constraintsFor(VersionConstraint constraint) {
On 2015/05/05 20:49:09, Bob Nystrom wrote:
> Make this a function?

What do you mean?

https://codereview.chromium.org/1127783002/diff/20001/test/version_range_test...
File test/version_range_test.dart (right):

https://codereview.chromium.org/1127783002/diff/20001/test/version_range_test...
test/version_range_test.dart:176: expect(range.allowsAll(range), isTrue);
On 2015/05/05 20:49:09, Bob Nystrom wrote:
> Maybe test against new VersionRange() (i.e. the open range).

Done.

https://codereview.chromium.org/1127783002/diff/20001/test/version_range_test...
test/version_range_test.dart:373: expect(result, allows(v123));
On 2015/05/05 20:49:09, Bob Nystrom wrote:
> To match the next tests, maybe put a doesNotAllow here for the gap between the
> two?

Done.

https://codereview.chromium.org/1127783002/diff/20001/test/version_union_test...
File test/version_union_test.dart (right):

https://codereview.chromium.org/1127783002/diff/20001/test/version_union_test...
test/version_union_test.dart:12: group('factory', () {
On 2015/05/05 20:49:09, Bob Nystrom wrote:
> Test an empty list.

Done.

[Bob Nystrom, 2015-05-06 17:30:58]

Don't forget to tag and publish!

LGTM!

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_union.dart
File lib/src/version_union.dart (right):

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_union.d...
lib/src/version_union.dart:167: List<VersionConstraint>
_constraintsFor(VersionConstraint constraint) {
On 2015/05/05 22:52:34, nweiz wrote:
> On 2015/05/05 20:49:09, Bob Nystrom wrote:
> > Make this a function?
> 
> What do you mean?

It's an instance method here, but it doesn't access any fields. You could just
hoist it out to a top level function to make that clear.

https://codereview.chromium.org/1127783002/diff/40001/lib/src/version_range.dart
File lib/src/version_range.dart (right):

https://codereview.chromium.org/1127783002/diff/40001/lib/src/version_range.d...
lib/src/version_range.dart:164: //     [  other  ]        [  other  ]
<3 the art.

[nweiz, 2015-05-06 18:18:58]

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_union.dart
File lib/src/version_union.dart (right):

https://codereview.chromium.org/1127783002/diff/20001/lib/src/version_union.d...
lib/src/version_union.dart:167: List<VersionConstraint>
_constraintsFor(VersionConstraint constraint) {
On 2015/05/06 17:30:58, Bob Nystrom wrote:
> On 2015/05/05 22:52:34, nweiz wrote:
> > On 2015/05/05 20:49:09, Bob Nystrom wrote:
> > > Make this a function?
> > 
> > What do you mean?
> 
> It's an instance method here, but it doesn't access any fields. You could just
> hoist it out to a top level function to make that clear.

We don't typically do this for private methods in classes, even those that don't
access state.

My feeling is that it's more valuable to be able to easily change a method to
make it use state than it is to clearly signal that it currently doesn't.

[nweiz, 2015-05-06 18:19:15]

Committed patchset #3 (id:40001) manually as
70bb15f1004b40fff934286c829cc5b54aa04e86 (presubmit successful).