Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for range and comparators comparison #187

Closed
wants to merge 5 commits into from
Closed

Add support for range and comparators comparison #187

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a29e8db
Add support for range and comparators comparison
rtfpessoa Apr 1, 2017
b5504e7
Move methods to comparator and range
rtfpessoa Apr 1, 2017
57c77f3
Add range and comparator intersection docs to README
rtfpessoa Apr 9, 2017
0cb989f
Reduce line length and remove unused platform parameter
rtfpessoa Jul 15, 2017
4fa71d9
Make exposed apis more clear
rtfpessoa Jul 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@
.*.swp
coverage/
.nyc_output/
.idea
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

54 changes: 54 additions & 0 deletions semver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -704,6 +704,48 @@ Comparator.prototype.test = function(version) {
return cmp(version, this.operator, this.semver, this.loose);
};

Comparator.prototype.intersects = function(comp, loose, platform) {
if (!(comp instanceof Comparator)) {
throw new TypeError('a Comparator is required');
}

var rangeTmp;

if (this.operator === '') {
rangeTmp = new Range(comp.value, loose, platform);
return satisfies(this.value, rangeTmp, loose, platform);
} else if (comp.operator === '') {
rangeTmp = new Range(this.value, loose, platform);
return satisfies(comp.semver, rangeTmp, loose, platform);
}

var sameDirectionIncreasing = (this.operator === '>=' || this.operator === '>') && (comp.operator === '>=' || comp.operator === '>');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

image

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var differentDirectionsInclusive = (this.operator === '>=' || this.operator === '<=') && (comp.operator === '>=' || comp.operator === '<=');
var oppositeDirectionsLessThan = cmp(this.semver, '<', comp.semver, loose) &&
((this.operator === '>=' || this.operator === '>') && (comp.operator === '<=' || comp.operator === '<'));
var oppositeDirectionsGreaterThan = cmp(this.semver, '>', comp.semver, loose) &&
((this.operator === '<=' || this.operator === '<') && (comp.operator === '>=' || comp.operator === '>'));

return sameDirectionIncreasing || sameDirectionDecreasing || (sameSemVer && differentDirectionsInclusive) ||
oppositeDirectionsLessThan || oppositeDirectionsGreaterThan;
};

Comparator.prototype.satisfiesRange = function(range, loose, platform) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (!(range instanceof Range)) {
throw new TypeError('a Range is required');
}

var comp = this;

return range.set.some(function(comparators) {
return comparators.every(function(comparator) {
return comp.intersects(comparator, loose, platform);
});
});
};


exports.Range = Range;
function Range(range, loose) {
Expand Down Expand Up @@ -783,6 +825,18 @@ Range.prototype.parseRange = function(range) {
return set;
};

Range.prototype.intersects = function(range, loose, platform) {
if (!(range instanceof Range)) {
throw new TypeError('a Range is required');
}

return this.set.some(function(comparators) {
return comparators.every(function(comparator) {
return comparator.satisfiesRange(range, loose, platform);
});
});
};

// Mostly just for testing and legacy API reasons
exports.toComparators = toComparators;
function toComparators(range, loose) {
Expand Down
105 changes: 105 additions & 0 deletions test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ var replaceStars = semver.replaceStars;
var toComparators = semver.toComparators;
var SemVer = semver.SemVer;
var Range = semver.Range;
var Comparator = semver.Comparator;

test('\ncomparison tests', function(t) {
// [version1, version2]
Expand Down Expand Up @@ -712,3 +713,107 @@ test('\nmin satisfying', function(t) {
});
t.end();
});

test('\nintersect comparators', function(t) {
[
// One is a Version
['1.3.0', '>=1.3.0', true],
['1.3.0', '>1.3.0', false],
['>=1.3.0', '1.3.0', true],
['>1.3.0', '1.3.0', false],
// Same direction increasing
['>1.3.0', '>1.2.0', true],
['>1.2.0', '>1.3.0', true],
['>=1.2.0', '>1.3.0', true],
['>1.2.0', '>=1.3.0', true],
// Same direction decreasing
['<1.3.0', '<1.2.0', true],
['<1.2.0', '<1.3.0', true],
['<=1.2.0', '<1.3.0', true],
['<1.2.0', '<=1.3.0', true],
// Different directions, same semver and inclusive operator
['>=1.3.0', '<=1.3.0', true],
['>=1.3.0', '>=1.3.0', true],
['<=1.3.0', '<=1.3.0', true],
['>1.3.0', '<=1.3.0', false],
['>=1.3.0', '<1.3.0', false],
// Opposite matching directions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

['>1.0.0', '<2.0.0', true],
['>=1.0.0', '<2.0.0', true],
['>=1.0.0', '<=2.0.0', true],
['>1.0.0', '<=2.0.0', true],
['<=2.0.0', '>1.0.0', true],
['<=1.0.0', '>=2.0.0', false]
].forEach(function(v) {
var comparator1 = new Comparator(v[0]);
var comparator2 = new Comparator(v[1]);
var expect = v[2];

var actual1 = comparator1.intersects(comparator2);
var actual2 = comparator2.intersects(comparator1);
t.equal(actual1, expect);
t.equal(actual2, expect);
});
t.end();
});

test('\nmissing comparator parameter in intersect comparators', function(t) {
t.throws(function() {
new Comparator('>1.0.0').intersects();
}, new TypeError('a Comparator is required'),
'throws type error');
t.end();
});

test('\ncomparator satisfies range', function(t) {
[
['1.3.0', '1.3.0 || <1.0.0 >2.0.0', true],
['1.3.0', '<1.0.0 >2.0.0', false],
['>=1.3.0', '<1.3.0', false],
['<1.3.0', '>=1.3.0', false]
].forEach(function(v) {
var comparator = new Comparator(v[0]);
var range = new Range(v[1]);
var expect = v[2];
var actual = comparator.satisfiesRange(range);
t.equal(actual, expect);
});
t.end();
});

test('\nmissing range parameter in comparator satisfies range', function(t) {
t.throws(function() {
new Comparator('>1.0.0').satisfiesRange();
}, new TypeError('a Range is required'),
'throws type error');
t.end();
});

test('\nranges intersect', function(t) {
[
['1.3.0 || <1.0.0 >2.0.0', '1.3.0 || <1.0.0 >2.0.0', true],
['<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.0.0', true],
['<1.0.0 >=2.0.0', '2.1.0', false],
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

['<1.0.0 >=2.0.0', '>1.4.0 <1.6.0 || 2.0.0', false]
].forEach(function(v) {
var range1 = new Range(v[0]);
var range2 = new Range(v[1]);
var expect = v[2];
var actual1 = range1.intersects(range2);
var actual2 = range2.intersects(range1);
t.equal(actual1, expect);
t.equal(actual2, expect);
});
t.end();
});

test('\nmissing range parameter in range intersect', function(t) {
t.throws(function() {
new Range('1.0.0').intersects();
}, new TypeError('a Range is required'),
'throws type error');
t.end();
});