-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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.
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
loosely detect semver versions #1628
Conversation
Generated by 🚫 dangerJS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I haven't concluded whether or not this is the right thing to do, though I thought I would nudge this forward with a review comment.
lib/version.js
Outdated
@@ -18,11 +18,13 @@ function latest(versions) { | |||
return (/[0-9]/).test(version); | |||
}); | |||
try { | |||
version = semver.maxSatisfying(versions, ''); | |||
version = semver.maxSatisfying(versions, '', true) || versions.sort((a, b) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we write these calls like maxSatisfying(versions, /* range */ '', /* loose */ true)
to make it clearer what these parameters are? Same for the other uses of the loose
parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have updated to include the /* loose */ true
comments, but it does look a little messy.
Do you think it would be better to do something like:
const semverLoose = true;
version = maxSatisfying(versions, /* range */ '', semverLoose);
const loose = true;
const range = '';
version = maxSatisfying(versions, range, loose);
Updated to first only compare all "semver" versions ( both could still fail if there are any strange versions like If that fails then it will just be alphabetically sorted as before. Edit: this also closes #1652 |
comments refered to incorrect test
Updated to also include a fix for #1682 |
Updated to make |
lib/version.js
Outdated
@@ -11,18 +11,31 @@ const semver = require('semver'); | |||
|
|||
// Given a list of versions (as strings), return the latest version. | |||
// Return undefined if no version could be found. | |||
function latest(versions) { | |||
function latest(versions, pre = false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make this an object?
function latest(versions, { pre = false } = {}) {
That way, when it's invoked, the parameter can be named, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated!
👍
lib/version.js
Outdated
if (!pre){ | ||
versions = versions.filter(function(version) { | ||
return !(/\d+-\w+/).test(version); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit difficult to follow.
I understand the purpose served by filtering out the prereleases. Could you explain what the rest of the filtering is for? Would it not work to just pass the other versions to the loose compare function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the \d+\.\d+
test first to get what we would assume is definitely a version.
If that returns no results, we fall back to just look for anything that could possibly be a version.
Otherwise in an array of:
["abc123f312", "1.2.3", "1.3"]
abc123f312
would be the newest version as it's interpreted as 123
var format = match[2]; | ||
var apiUrl = 'https://pub.dartlang.org/packages/' + userRepo + '.json'; | ||
var badgeData = getBadgeData('pub', data); | ||
const includePre = Boolean(match[1]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Think this should be ready for merge now! |
Closes #1622
Closes #1652
Closes #1682
refer: https://www.npmjs.com/package/semver#functions
This PR will allow "not-quite-valid semver strings" example:
1.023.4
,1.23
Note:
Only the change on line 21 is needed to close #1622, but figured we should add support for the other places these functions are used also.
Note2/Edit: