Add support for range and comparators comparison #187

rtfpessoa · 2017-04-01T14:15:44Z

Check if two ranges intersect
Check if one comparator satisfies a range
Check if two comparators intersect

Fixes #178

* Check if two ranges intersect * Check if one comparator satisfies a range * Check if two comparators intersect

shellscape · 2017-04-01T15:06:48Z

semver.js

+    ((compA.operator === '<=' || compA.operator === '<') && (compB.operator === '>=' || compB.operator === '>')));
+}
+
+exports.comparatorSatisfiesRange = comparatorSatisfiesRange;


function names are a little overly verbose/long for my tastes. but looks like good functionality

Similarly; if this was a prototype method on Comparator it could just be satisfiesRange?

Sure, I think I was following the toComparators logic but I think I like it more this way too.

rtfpessoa · 2017-04-01T15:11:59Z

@shellscape I am completely open for alternatives. Let me know if you have any suggestions.

ljharb · 2017-04-01T16:52:45Z

semver.js

@@ -793,6 +793,56 @@ function toComparators(range, loose) {
  });
 }

+exports.comparatorsIntersect = comparatorsIntersect;
+function comparatorsIntersect(compA, compB, loose, platform) {


Is this better as a standalone function, or as a prototype method on Comparator?

ljharb · 2017-04-01T16:53:03Z

semver.js

+exports.comparatorsIntersect = comparatorsIntersect;
+function comparatorsIntersect(compA, compB, loose, platform) {
+  compA = new Comparator(compA, loose);
+  compB = new Comparator(compB, loose);


Reassigning function arguments deopts in v8; could you use new variables for these?

ljharb · 2017-04-01T16:53:36Z

semver.js

+    ((compA.operator === '<=' || compA.operator === '<') && (compB.operator === '>=' || compB.operator === '>')));
+}
+
+exports.comparatorSatisfiesRange = comparatorSatisfiesRange;


Similarly; if this was a prototype method on Comparator it could just be satisfiesRange?

ljharb · 2017-04-01T16:54:24Z

semver.js

+}
+
+exports.rangesIntersect = rangesIntersect;
+function rangesIntersect(rangeA, rangeB, loose, platform) {


And if this was a prototype method on Range it could just be intersects?

ljharb · 2017-04-01T16:55:10Z

test/index.js

+    ['<1.0.0 >2.0.0', '>0.0.0', true],
+    ['<1.0.0 >2.0.0', '>1.4.0 <1.6.0', false],
+    ['<1.0.0 >2.0.0', '>1.4.0 <1.6.0 || 2.0.0', false],
+    ['<1.0.0 >=2.0.0', '2.1.0', false],


Could you add a test case for a single version returning true?

ljharb · 2017-04-01T16:55:38Z

test/index.js

+    var range2 = v[1];
+    var expect = v[2];
+    var actual = semver.rangesIntersect(range1, range2);
+    t.equal(actual, expect);


Could we also assert that if you flip the range order that it provides the same value?

ljharb · 2017-04-01T16:56:16Z

test/index.js

+    ['<=1.3.0', '<=1.3.0', true],
+    ['>1.3.0', '<=1.3.0', false],
+    ['>=1.3.0', '<1.3.0', false],
+    // Opposite matching directions


This could instead be done in the iterator function, for every test, no?

I am not understanding exactly what you mean here. Can you extend a bit?

Also, should I amend my previous commit or should I create new commits to fix the requested changes?

I mean, line 752 asks if comparator1 intersects comparator2; could you also add an assertion after that asking if comparator2 intersects comparator1?

I think amending and force pushing is always fine; unless one of this repo's collabs says different.

ljharb · 2017-04-01T23:57:26Z

semver.js

@@ -704,6 +704,42 @@ Comparator.prototype.test = function(version) {
  return cmp(version, this.operator, this.semver, this.loose);
 };

+Comparator.prototype.intersects = function(compOrStr, loose, platform) {
+  var rangeTmp;
+  var comp = new Comparator(compOrStr, loose);


Would it be bad if this only took a Comparator? That would make the API more explicit and delegate more decisions to the user.

I am ok with this change, but I actually do not think the user makes more decisions. Since we allow the method to receive either a comparator or a string they can do all the same decisions if they want.

But the API is definitely more explicit if we force a Comparator.

Usually I am a bit more defensive and that is why I usually try to handle all the weird possibilities. But if this was Typescript it would make much more sense to receive only the comparator.

I don't think typescript has anything to do with it; it's nicer to have monomorphic APIs with or without types. By "decisions" I mean, the comparator and the range having the same loose setting, for example - forcing the user to make that choice means they can never be ignorant of the default.

ljharb · 2017-04-01T23:57:35Z

semver.js

+
+Comparator.prototype.satisfiesRange = function(rangeOrStr, loose, platform) {
+  var comp = this;
+  var range = new Range(rangeOrStr, loose, platform);


Same question here

ljharb · 2017-04-01T23:57:43Z

semver.js

@@ -783,6 +819,16 @@ Range.prototype.parseRange = function(range) {
  return set;
 };

+Range.prototype.intersects = function(rangeOrStr, loose, platform) {
+  var range = new Range(rangeOrStr, loose, platform);


ljharb · 2017-04-01T23:58:42Z

test/index.js

+    ['<=1.3.0', '<=1.3.0', true],
+    ['>1.3.0', '<=1.3.0', false],
+    ['>=1.3.0', '<1.3.0', false],
+    // Opposite matching directions


I mean, line 752 asks if comparator1 intersects comparator2; could you also add an assertion after that asking if comparator2 intersects comparator1?

ljharb · 2017-04-01T23:59:08Z

test/index.js

+    ['<=1.3.0', '<=1.3.0', true],
+    ['>1.3.0', '<=1.3.0', false],
+    ['>=1.3.0', '<1.3.0', false],
+    // Opposite matching directions


I think amending and force pushing is always fine; unless one of this repo's collabs says different.

rtfpessoa · 2017-04-02T13:24:38Z

@ljharb I think I made all the changes you suggested in this last commit. Take a look when you have some time.

ljharb

I think this looks great!

I would strongly prefer much more defensive throwing to check the types of arguments at runtime, but il leave insisting on that to the collaborators.

Hopefully this gets merged and released soon!

ljharb · 2017-04-02T17:30:50Z

semver.js

+    return satisfies(comp.semver, rangeTmp, loose, platform);
+  }
+  // Same direction increasing
+  return ((this.operator === '>=' || this.operator === '>') && (comp.operator === '>=' || comp.operator === '>')) ||


I feel like making this one statement makes it a bit more confusing to read; what do you think about using if/else here?

Can you provide an example?

Sure - maybe just separate variables would help. Here's a subset:

var sameDirectionIncreasing = (this.operator === '>=' || this.operator === '>') && (comp.operator === '>=' || comp.operator === '>'); var sameDirectionDecreasing = (this.operator === '<=' || this.operator === '<') && (comp.operator === '<=' || comp.operator === '<'); var sameSemver = this.semver.raw === comp.semver.raw; var differentDirectionsInclusive = (this.operator === '>=' || this.operator === '<=') && (comp.operator === '>=' || comp.operator === '<='); return sameDirectionIncreasing || sameDirectionDecreasing || (sameSemver && differentDirectionsInclusive);

something like that? the long chain of boolean operations makes it hard for me to read.

Sounds like a nice improvement.

rtfpessoa · 2017-04-02T20:14:14Z

When you say "I would strongly prefer much more defensive throwing to check the types of arguments at runtime, but il leave insisting on that to the collaborators", like what check if instanceof?

ljharb · 2017-04-02T21:48:13Z

@rtfpessoa exactly like that - i'd do if (!(range instanceof Range)) { throw new TypeError('a Range is required'); }, if (typeof loose !== 'undefined' || typeof loose !== 'boolean') { throw new TypeError('loose must be a boolean if present'); } and things like that.

In other words, if I'm passing the wrong kind of argument, I want to be alerted to that fact as soon and explicitly as possible.

rtfpessoa · 2017-04-02T22:07:09Z

Just did both changes except for the loose and platform check since I never use them and for that reason it would make more sense to be checked in Range or Comparator.

rtfpessoa · 2017-04-02T22:20:41Z

Also, I added a couple more tests for the parameter checks and fixed the format issues.

coveralls · 2017-04-02T22:31:37Z

Coverage increased (+0.2%) to 94.872% when pulling b5504e7 on rtfpessoa:master into 8fff305 on npm:master.

ljharb

Thanks! Much cleaner imo :-)

iarna · 2017-04-09T01:32:38Z

This seems super useful! It just needs documentation to land.

ljharb · 2017-04-09T01:42:49Z

Awesome! If @rtfpessoa can't find time to write it soon, I'll try to find time to write some :-)

rtfpessoa · 2017-04-09T14:29:45Z

@iarna can you extend a bit on what do you mean by documentation?
I could not find any docs explaining a proper contribution, maybe I looked in the wrong places. Can you point me in the right direction?

Edit: Do you mean README docs? If that is it, I completely missed it, will do it now.

coveralls · 2017-04-09T15:23:25Z

Coverage increased (+0.2%) to 94.872% when pulling 57c77f3 on rtfpessoa:master into 8fff305 on npm:master.

ljharb · 2017-04-09T17:16:30Z

.gitignore

@@ -2,3 +2,4 @@
 .*.swp
 coverage/
 .nyc_output/
+.idea


Generally this belongs in a global git config, rather than polluting specific git repos with IDE-specific knowledge (nbd tho)

iarna · 2017-04-12T20:54:36Z

@rtfpessoa Yes, that's exactly what I was looking for!

rtfpessoa · 2017-04-13T09:19:14Z

@iarna, not sure if you noticed but I already did it. Le me know if you need any other changes for the merge.

rtfpessoa · 2017-05-03T10:25:33Z

any updates about this @isaacs @iarna ?

franher · 2017-06-01T08:51:44Z

looking forward for the landing of this PR 🙏

agebhardt · 2017-06-28T20:16:40Z

Really excited to be able to use this feature! Are there any updates on this PR?

isaacs

This seems like it works, and Range intersection is definitely something that people ask for often. Thanks for taking the lead on this, and sorry for the delays in reviewing.

Some changes requested, but I think we can get this landed relatively quickly. Thanks!

isaacs · 2017-07-08T16:08:11Z

semver.js

+    return satisfies(comp.semver, rangeTmp, loose, platform);
+  }
+
+  var sameDirectionIncreasing = (this.operator === '>=' || this.operator === '>') && (comp.operator === '>=' || comp.operator === '>');


These really long lines are very hard to read.

Can you please break this up into separate stanzas? There should be no lines longer than 80 chars under any circumstances. 70 is an even better limit.

For more context on this, I've got somewhat bad vision, and usually keep my font size turned way up. It's a pretty common accessibility issue, though.

isaacs · 2017-07-08T16:09:22Z

semver.js

+    oppositeDirectionsLessThan || oppositeDirectionsGreaterThan;
+};
+
+Comparator.prototype.satisfiesRange = function(range, loose, platform) {


What is the "platform" variable being passed around here? It doesn't seem like it's ever used for anything.

Ported this from the Ruby version and somehow did not notice platform did not exist here.

isaacs · 2017-07-08T16:16:19Z

README.md

@@ -329,6 +329,10 @@ strings that they parse.
  (`major`, `premajor`, `minor`, `preminor`, `patch`, `prepatch`, or `prerelease`),
  or null if the versions are the same.

+### Comparators
+* `intersects(comparator)`: Return true if the comparators intersect
+* `satisfiesRange(range)`: Return true if the comparator intersects with any


The naming of this is a bit weird. You wouldn't normally say that a comparator "satisfies" a range. A range is satisfied by a single thing which has a specific version, not by a comparator, which is like a mini-range. Also, a comparator like <=1.4.5 would return true for this method, given the Range >=1.3.0, even though many versions allowed by that comparator are not satisfying answers for the Range. We can only talk about intersections and unions with ranges (and their building blocks, Comparators) because they are sets, not discrete atomic items.

Having a comparator.intersects(comparator) method seems like a building block towards having range intersection. People generally don't interact with comparators directly. Having (effectively) comparator.intersects(range) is a bit odd, and using the language of "satisfying" makes it odder.

It seems like what you'd really want in most cases is range.intersects(range). It's probably better to leave aside things that don't get us closer to that.

I think this is probably addressed by calling it comparator.intersectsRange()? Or even cleaner, make Comparator.intersects take either a Range or Comparator as an argument.

rtfpessoa · 2017-07-13T12:51:25Z

@isaacs will try to do this soon.

rtfpessoa · 2017-07-15T23:32:00Z

@isaacs let me know if this was what you asked. I removed the confusing method and exposed only the two intersections between the same type.

coveralls · 2017-07-15T23:32:32Z

Coverage increased (+0.2%) to 94.835% when pulling 4fa71d9 on rtfpessoa:master into 8fff305 on npm:master.

ljharb · 2017-07-23T07:02:34Z

@isaacs @iarna @zkat any update on this one? I've been waiting on its release to fix an eslint bug for about 9 months now :-/

isaacs · 2017-07-24T16:01:29Z

semver.js

+  var sameDirectionDecreasing =
+    (this.operator === '<=' || this.operator === '<') &&
+    (comp.operator === '<=' || comp.operator === '<');
+  var sameSemVer = this.semver.raw === comp.semver.raw;


I think what you're after isn't semver.raw here, but semver.version. Consider this:

> r2 = new s.Range('>=v1.2.3') Range { loose: undefined, raw: '>=v1.2.3', set: [ [ [Object] ] ], range: '>=1.2.3' } > r = new s.Range('<=1.2.3') Range { loose: undefined, raw: '<=1.2.3', set: [ [ [Object] ] ], range: '<=1.2.3' } > r.intersects(r2) false

Yep, that fixes it. I'll include it in the merge, no worries.

isaacs · 2017-07-24T16:40:17Z

Landed on master and published as v5.4.0. Also added a top-level semver.intersects(r1, r2) command, and a bunch of tests to improve coverage.

ljharb · 2017-07-24T17:48:19Z

Yay, thanks!

Add support for range and comparators comparison

a29e8db

* Check if two ranges intersect * Check if one comparator satisfies a range * Check if two comparators intersect

isaacs added the review label Apr 1, 2017

shellscape reviewed Apr 1, 2017

View reviewed changes

ljharb suggested changes Apr 1, 2017

View reviewed changes

rtfpessoa force-pushed the master branch from 3977a17 to a6eb60a Compare April 1, 2017 22:33

ljharb reviewed Apr 1, 2017

View reviewed changes

rtfpessoa force-pushed the master branch from a6eb60a to a0f44a9 Compare April 2, 2017 13:23

ljharb approved these changes Apr 2, 2017

View reviewed changes

rtfpessoa mentioned this pull request Apr 2, 2017

Add support for range and comparators comparison librariesio/semantic_range#62

Merged

rtfpessoa force-pushed the master branch from a0f44a9 to 40b7599 Compare April 2, 2017 22:05

rtfpessoa force-pushed the master branch 2 times, most recently from 5da850d to 7a525c5 Compare April 2, 2017 22:20

Move methods to comparator and range

b5504e7

rtfpessoa force-pushed the master branch from 7a525c5 to b5504e7 Compare April 2, 2017 22:30

ljharb approved these changes Apr 2, 2017

View reviewed changes

Add range and comparator intersection docs to README

57c77f3

ljharb approved these changes Apr 9, 2017

View reviewed changes

rtfpessoa mentioned this pull request May 16, 2017

calculate two Ranges intersection. #195

Closed

isaacs suggested changes Jul 8, 2017

View reviewed changes

rtfpessoa added 2 commits July 16, 2017 00:19

Reduce line length and remove unused platform parameter

0cb989f

Make exposed apis more clear

4fa71d9

isaacs reviewed Jul 24, 2017

View reviewed changes

isaacs closed this Jul 24, 2017

Add support for range and comparators comparison #187

Add support for range and comparators comparison #187

Conversation

rtfpessoa commented Apr 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtfpessoa commented Apr 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtfpessoa commented Apr 2, 2017

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtfpessoa commented Apr 2, 2017

ljharb commented Apr 2, 2017

rtfpessoa commented Apr 2, 2017

rtfpessoa commented Apr 2, 2017

coveralls commented Apr 2, 2017 • edited Loading

ljharb left a comment

Choose a reason for hiding this comment

iarna commented Apr 9, 2017

ljharb commented Apr 9, 2017 • edited Loading

rtfpessoa commented Apr 9, 2017 • edited Loading

coveralls commented Apr 9, 2017 • edited Loading

Choose a reason for hiding this comment

iarna commented Apr 12, 2017

rtfpessoa commented Apr 13, 2017

rtfpessoa commented May 3, 2017

franher commented Jun 1, 2017

agebhardt commented Jun 28, 2017

isaacs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtfpessoa commented Jul 13, 2017

rtfpessoa commented Jul 15, 2017

coveralls commented Jul 15, 2017 • edited Loading

ljharb commented Jul 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaacs commented Jul 24, 2017

ljharb commented Jul 24, 2017

coveralls commented Apr 2, 2017 •

edited

Loading

ljharb commented Apr 9, 2017 •

edited

Loading

rtfpessoa commented Apr 9, 2017 •

edited

Loading

coveralls commented Apr 9, 2017 •

edited

Loading

coveralls commented Jul 15, 2017 •

edited

Loading