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
fix version sorting for ruby [gem] s #8434
Conversation
function latest(versions) { | ||
// latest Ruby Gems version, including pre-releases | ||
return maxSatisfying(versions, '>0') | ||
} |
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 haven't implemented including/excluding pre-releases here - for stable version only we just report the .version
param from the API so we only actually need the 'including pre-releases' case.
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.
Do you know offhand if this will support or typical behavior of falling back to a prerelease version in cases where the user has requested stable-only versions but all the versions are prerelease?
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.
As I say, I haven't implemented includePre
here because we don't need to use this function if the user wants latest stable.
If we want the latest stable, we just return the value of .version
here:
shields/services/gem/gem-version.service.js
Lines 71 to 73 in d8c3dd1
} else { | |
const { version } = await this.fetch({ gem }) | |
return this.constructor.render({ version }) |
maxSatisfying()
doesn't care about stable/pre-releases. It just returns the latest version that matches the constraint, so if we wanted to implement a version that returns the latest stable, we'd do it something like (not actually tested, but hopefully you get the point):
function latest(versions, includePre) {
const stableVersions = versions.filter((v) => !prerelease(v) && v[0] !== '0')
if (stableVersions.length >0 && !includePre) {
return maxSatisfying(stableVersions, '>0')
}
return maxSatisfying(versions, '>0')
}
but we don't need it
so I haven't bothered
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.
got it. apologies it wasn't clear to me on first read what you meant in the initial comment but i see it now 👍
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.
no probs
import { test, given } from 'sazerac' | ||
import { latest, versionColor } from './gem-helpers.js' | ||
|
||
describe('Gem helpers', 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 haven't written loads of test cases here. For the most part I think we can rely on the tests for the underlying library in https://github.com/renovatebot/ruby-semver/blob/main/test/index.test.ts I have covered a handful of cases, especially specific to the sorting of pre/stable releases here
closes #8425
As noted in #8425 our behaviour when sorting ruby gems with pre-releases is incorrect.
This is because we're attempting to use semver sorting, which assumes the pre-release identifier is separated from the main part of the version number by a
-
, rather than a.
. This PR switches us to using https://www.npmjs.com/package/@renovatebot/ruby-semver to implement ruby ecosystem-specific version sort/color logic instead of the generic version functions.Related docs: https://guides.rubygems.org/patterns/#prerelease-gems
I'm really surprised this has never come up before given how long these gem badges have been around, but lets get it fixed now.