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.

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

feat: PVP Version implementation for the Stack build tool (Haskell) #13224

Closed
wants to merge 25 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
3f66759
feat: Initial attempt at PVP implementation
psilospore Dec 21, 2021
ee79cf7
Initial sub at test
psilospore Dec 21, 2021
e0ca9a3
Merge remote-tracking branch 'upstream/main' into haskell-stack
psilospore Jan 16, 2022
50f5f46
Refactored to use GenericVersioningApi
psilospore Jan 17, 2022
ca70458
Update pvp to use regex
psilospore Jan 20, 2022
f587061
Update test case
psilospore Jan 20, 2022
cc7d212
Merge branch 'main' into haskell-stack
psilospore Jan 20, 2022
8f68b38
Apply suggestions from code review
psilospore Jan 20, 2022
0d5147e
Add documentation for PVP
psilospore Jan 26, 2022
d4f4f4b
Merge branch 'haskell-stack' of github.com:psilospore/renovate into h…
psilospore Jan 26, 2022
f16c946
Apply suggestions from code review
psilospore Jan 26, 2022
f9cd49a
Clarify optional additional components
psilospore Jan 26, 2022
d99e888
Update lib/versioning/pvp/readme.md
psilospore Jan 27, 2022
a913697
Merge branch 'main' into haskell-stack
viceice Jan 28, 2022
a582f6f
Apply suggestions from code review
psilospore Jan 28, 2022
fbd784d
fix: Added additional test coverage, return correct major version
psilospore Feb 10, 2022
53e4336
Merge branch 'main' into haskell-stack
psilospore Feb 10, 2022
41131a2
Merge branch 'main' into haskell-stack
rarkins Feb 15, 2022
f4ec4f3
fix: lint markdown and use import type
psilospore Feb 17, 2022
83d46c6
Merge branch 'haskell-stack' of github.com:psilospore/renovate into h…
psilospore Feb 17, 2022
971a3f8
Merge branch 'main' into haskell-stack
rarkins Feb 17, 2022
db52252
Apply suggestions from code review
psilospore Feb 21, 2022
d432f06
Merge branch 'main' into haskell-stack
psilospore Feb 21, 2022
5401d90
Merge branch 'main' into haskell-stack
rarkins Feb 22, 2022
78636d4
Merge branch 'main' into haskell-stack
rarkins Mar 4, 2022
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
2 changes: 2 additions & 0 deletions lib/modules/versioning/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import * as npm from './npm';
import * as nuget from './nuget';
import * as pep440 from './pep440';
import * as poetry from './poetry';
import * as pvp from './pvp';
import * as regex from './regex';
import * as rez from './rez';
import * as ruby from './ruby';
Expand Down Expand Up @@ -46,6 +47,7 @@ api.set('npm', npm.api);
api.set('nuget', nuget.api);
api.set('pep440', pep440.api);
api.set('poetry', poetry.api);
api.set('pvp', pvp.api);
api.set('regex', regex.api);
api.set('rez', rez.api);
api.set('ruby', ruby.api);
Expand Down
97 changes: 97 additions & 0 deletions lib/versioning/pvp/index.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
import { api as pvp } from '.';

describe('versioning/pvp/index', () => {
test.each`
version | isValid
${'0.1.0'} | ${true}
${'1.0.0'} | ${true}
${'0.0.1'} | ${true}
${'1.1.0.1.5.123455'} | ${true}
${'1.2.3'} | ${true}
${'1.02.3'} | ${false}
${'1.04'} | ${false}
${'1.2.3-foo'} | ${false}
${'1.2.3foo'} | ${false}
${'~1.2.3'} | ${false}
${'^1.2.3'} | ${false}
${'>1.2.3'} | ${false}
${'one.two.three'} | ${false}
${'renovatebot/renovate'} | ${false}
${'renovatebot/renovate#main'} | ${false}
${'https://github.com/renovatebot/renovate.git'} | ${false}
`('isValid("$version") === $isValid', ({ version, isValid }) => {
const res = pvp.isValid(version);
Copy link
Member

Choose a reason for hiding this comment

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

please inline

expect(res).toBe(isValid);
});

test.each`
input | expected
${'9.0.3'} | ${true}
${'1.2019.3.22'} | ${true}
${'3.0.0-beta'} | ${false}
${'2.0.2-pre20191018090318'} | ${false}
${'1.0.0+c30d7625'} | ${false}
${'2.3.4-beta+1990ef74'} | ${false}
${'17.04'} | ${false}
${'3.0.0.beta'} | ${false}
${'5.1.2-+'} | ${false}
`('isVersion("$input") === $expected', ({ input, expected }) => {
const res = !!pvp.isVersion(input);
Copy link
Member

Choose a reason for hiding this comment

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

See above

Copy link
Member

Choose a reason for hiding this comment

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

☝️

expect(res).toBe(expected);
});

test.each`
input | expected
${'9.0.3'} | ${true}
${'1.2019.3.22'} | ${true}
${'3.0.0-beta'} | ${false}
${'2.0.2-pre20191018090318'} | ${false}
${'1.0.0+c30d7625'} | ${false}
${'2.3.4-beta+1990ef74'} | ${false}
`('isStable("$input") === $expected', ({ input, expected }) => {
expect(pvp.isStable(input)).toBe(expected);
});

test.each`
a | b | expected
${'17.4.0'} | ${'17.4.0'} | ${true}
${'1.4'} | ${'1.4.0'} | ${false}
${'1.0.110'} | ${'1.0.110.0'} | ${false}
${'1.0.0'} | ${'1.0.0'} | ${true}
`('equals($a, $b) === $expected', ({ a, b, expected }) => {
expect(pvp.equals(a, b)).toBe(expected);
});

test.each`
a | b | expected
${'2.4.2'} | ${'2.4.1'} | ${true}
${'1.9.0'} | ${'2.0.0'} | ${false}
${'1.9.0'} | ${'1.9.1'} | ${false}
`('isGreaterThan($a, $b) === $expected', ({ a, b, expected }) => {
expect(pvp.isGreaterThan(a, b)).toBe(expected);
});

test.each`
input | expected
${'1.2.3'} | ${1.2}
${'1.0.2'} | ${1}
`('getMajor("$input") === $expected', ({ input, expected }) => {
expect(pvp.getMajor(input)).toBe(expected);
});

test.each`
input | expected
${'1.2.3'} | ${3}
${'1.0.0'} | ${0}
`('getMinor("$input") === $expected', ({ input, expected }) => {
expect(pvp.getMinor(input)).toBe(expected);
});

test.each`
input | expected
${'1.2.3.4'} | ${4}
${'1.0.2'} | ${undefined}
`('getPatch("$input") === $expected', ({ input, expected }) => {
expect(pvp.getPatch(input)).toBe(expected);
});
});
84 changes: 84 additions & 0 deletions lib/versioning/pvp/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
import { regEx } from '../../util/regex';
import { GenericVersion, GenericVersioningApi } from '../generic';
import type { VersioningApi } from '../types';

export const id = 'pvp';
export const displayName = 'PVP';
export const urls = ['https://pvp.haskell.org'];
export const supportsRanges = false;

/**
* At least three components and no leading zeros
* https://pvp.haskell.org/#version-number
*/
const versionPattern = regEx(
/^((([0-9])|([1-9][0-9]*))\.){2,}(([0-9])|([1-9][0-9]*))$/
);

class PvpVersioningApi extends GenericVersioningApi {
/**
* PVP has two major components A.B
viceice marked this conversation as resolved.
Show resolved Hide resolved
* To keep compatibility with Renovate's versioning API we will treat it as a float
*/
protected _parse(version: string): GenericVersion | null {
const matches = versionPattern.exec(version);
if (!matches) {
return null;
}

const components = version.split('.');
const major = parseFloat(`${components[0]}.${components[1]}`);
const rest = components.slice(2).map((i) => parseInt(i, 10));
return { release: [major, ...rest] };
}

override getMajor(version: string): number | null {
Copy link
Member

Choose a reason for hiding this comment

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

why you need to override this?

const {
release: [major],
} = this._parse(version);
return major;
}

override getMinor(version: string): number | null {
const {
release: [, minor],
} = this._parse(version);
return minor;
}

override getPatch(version: string): number | null {
const { release } = this._parse(version);
return release[2];
}

/**
* Compare similar to GenericVersioningApi._compare implementation
* except 2.1.1.0 and 2.1.1 are not equivalent instead 2.1.1.0 > 2.1.1
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this now fixed in base class, so we can remove this overload?

Copy link
Member

Choose a reason for hiding this comment

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

☝️

*/
override _compare(version: string, other: string): number {
const left = this._parse(version);
const right = this._parse(other);

// istanbul ignore if
if (!(left && right)) {
return 1;
}

// support variable length compare
const length = Math.max(left.release.length, right.release.length);
for (let i = 0; i < length; i += 1) {
// 2.1.0 > 2.1
const part1 = left.release[i] ?? -1;
const part2 = right.release[i] ?? -1;
if (part1 !== part2) {
return part1 - part2;
}
}

return 0;
}
}

export const api: VersioningApi = new PvpVersioningApi();

export default api;
21 changes: 21 additions & 0 deletions lib/versioning/pvp/readme.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# PVP

## What type of versioning is used?

Quotes from the [Haskell PVP Specification](https://pvp.haskell.org/):

> A package version number **SHOULD** have the form A.B.C, and **MAY** optionally have any number of additional components, for example 2.1.0.4 (in this case, A=2, B=1, C=0).
> A.B is known as the _major_ version number, and C the _minor_ version number.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
> A.B is known as the _major_ version number, and C the _minor_ version number.
> A.B is known as the _major_ version number, and C the _minor_ version number.

These are separate quotes in the upstream docs, so we should put a newline between them.


The one unusual difference between PVP and SemVer is that there are two major versions, and that there can be an optional number of additional version numbers past _minor_.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
The one unusual difference between PVP and SemVer is that there are two major versions, and that there can be an optional number of additional version numbers past _minor_.
The main differences between PVP and SemVer:
- PVP allows _two_ major versions
- PVP allows an optional number of extra version numbers past _minor_

For example `1.2.3` (note there's no _patch_ version here), `1.2.3.4` (`4` is the _patch_ version), or `1.2.3.4.5.6.7` are all valid version numbers.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
For example `1.2.3` (note there's no _patch_ version here), `1.2.3.4` (`4` is the _patch_ version), or `1.2.3.4.5.6.7` are all valid version numbers.
| PVP version | Note |
| --- | --- |
| `1.2.3` | No _patch_ version |
| `1.2.3.4` | `4` is the _patch_ version |
| `1.2.3.4.5.6.7` | This is also a valid version |

We can try a table here. You'll need to run yarn prettier-fix after applying the suggestion because it's not formatted the way Prettier wants.


## Are ranges supported?

Currently this is not supported but you can have ranges.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The h2 asks: Are ranges supported?
And our answer is: this is not supported, but right after: but you can have ranges.

This is really confusing me. I think there's some kind of difference between Cabal and Stack?

This implementation just supports updating extra-deps in the Stack build tool which does not support ranges.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
This implementation just supports updating extra-deps in the Stack build tool which does not support ranges.
Our implementation supports updating extra-deps in the Stack build tool which does not support ranges.

We can probably say "our implementation" and get rid of the word "just". 😉

If this is used for Cabal then it would be useful to support ranges.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This sentence doesn't really explain if I can use ranges with Cabal or not currently?


## Are commit hashes supported?

No.