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

fix: incorrect results from diff sometimes with prerelease versions #546

Merged
merged 7 commits into from
Apr 13, 2023

Conversation

tjenkinson
Copy link
Contributor

@tjenkinson tjenkinson commented Apr 11, 2023

The following inputs previously gave the incorrect result based on my understanding:

  • 0.0.2-1 => 0.0.3 was prepatch when it should be patch, as it's doing a patch operation that goes from the first to the second. Fixed with first commit.
  • 0.0.0-1 => 0.0.0 was prerelease when it should be major. I think 0.0.0-1 => 0.0.0 should be consistent with 1.0.0-1 => 1.0.0. Fixed with second commit. This makes it more consistent but not sure if it's actually what people would expect. See below.
  • 1.0.0-1 => 2.0.0-1 was premajor but should be major because the biggest change is the major version. Fixed with third commit.

I'm actually not convinced the behaviour for the second point is correct. For any version change where the stable part remains the same and the prerelease gets removed, npm version patch would give that result. So should the diff always be patch for these cases?

I.e 1.1.0-pre => 1.1.0 is achievable with npm version patch and npm version minor. I personally would have expected the diff between those to be patch, just like it would be if it was 1.1.1-pre => 1.1.1

`0.0.2-1` => `0.0.3` should be `patch` not `prepatch`
I.e. `1.0.0-1` => `2.0.0-1` should be `major` not `premajor`, because the biggest change is the major version
@tjenkinson tjenkinson marked this pull request as ready for review April 11, 2023 22:37
@tjenkinson tjenkinson requested a review from a team as a code owner April 11, 2023 22:37
@tjenkinson tjenkinson requested a review from fritzy April 11, 2023 22:37
functions/diff.js Outdated Show resolved Hide resolved
@wraithgar
Copy link
Member

Yeah I think the pre* diff results are very counterintuitive. They are trying to leverage the operations that .inc would do, but diff is not currently directional. Your PR attempts to make the comparison directional which is the part that would need to be discussed.

functions/diff.js Outdated Show resolved Hide resolved
@wraithgar wraithgar changed the title Fix incorrect result from diff sometimes with prerelease versions fix: incorrect results from diff sometimes with prerelease versions Apr 12, 2023
@wraithgar wraithgar requested a review from nlf April 12, 2023 22:08
@wraithgar
Copy link
Member

Gonna kind of spam those test lines w/ comments sorry for the noise but this deserves a good paper trail

['0.0.2-1', '0.0.2', 'patch'],
['0.0.2-1', '0.0.3', 'patch'],
Copy link
Member

@wraithgar wraithgar Apr 12, 2023

Choose a reason for hiding this comment

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

The difference is actually two .inc patches, but that's still a patch difference. A good test

> semver.inc('0.0.2-1', 'patch')
'0.0.2'
> semver.inc('0.0.2', 'patch')
'0.0.3'

['0.0.2-1', '0.0.2', 'patch'],
['0.0.2-1', '0.0.3', 'patch'],
['0.0.2-1', '0.1.0', 'minor'],
Copy link
Member

Choose a reason for hiding this comment

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

> semver.inc('0.0.2-1', 'minor')
'0.1.0'

['0.0.2-1', '0.0.2', 'patch'],
['0.0.2-1', '0.0.3', 'patch'],
['0.0.2-1', '0.1.0', 'minor'],
['0.0.2-1', '1.0.0', 'major'],
Copy link
Member

Choose a reason for hiding this comment

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

> semver.inc('0.0.2-1', 'major')
'1.0.0'

@@ -19,10 +19,19 @@ test('diff versions test', (t) => {
['1.1.0', '1.1.0-pre', 'minor'],
['1.1.0-pre-1', '1.1.0-pre-2', 'prerelease'],
['1.0.0', '1.0.0', null],
['1.0.0-1', '1.0.0-1', null],
Copy link
Member

Choose a reason for hiding this comment

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

> semver.eq('1.0.0-1', '1.0.0-1')
true

['0.1.0-1', '0.1.0', 'minor'],
['1.0.0-1', '1.0.0', 'major'],
['0.0.0-1', '0.0.0', 'prerelease'],
['1.0.1-1', '1.0.1', 'patch'],
Copy link
Member

Choose a reason for hiding this comment

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

> semver.inc('1.0.1-1', 'patch')
'1.0.1'

['0.1.0-1', '0.1.0', 'minor'],
['1.0.0-1', '1.0.0', 'major'],
['0.0.0-1', '0.0.0', 'prerelease'],
['1.0.1-1', '1.0.1', 'patch'],
['0.0.0-1', '0.0.0', 'major'],
Copy link
Member

Choose a reason for hiding this comment

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

> semver.inc('0.0.0-1', 'major')
'0.0.0'

['0.0.0-1', '0.0.0', 'prerelease'],
['1.0.1-1', '1.0.1', 'patch'],
['0.0.0-1', '0.0.0', 'major'],
['1.0.0-1', '2.0.0', 'major'],
Copy link
Member

Choose a reason for hiding this comment

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

Again this is two major .inc operations but that's still a major diff

> semver.inc('1.0.0-1', 'major')
'1.0.0'
> semver.inc('1.0.0', 'major')
'2.0.0'

test/functions/diff.js Outdated Show resolved Hide resolved
@wraithgar
Copy link
Member

1.0.0-1 => 2.0.0-1 was premajor but should be major because the biggest change is the major version. Fixed with third commit.

This still seems like premajor as per my comment on the test fixture:

> semver.inc('1.0.0', 'premajor')
'2.0.0-0'

@tjenkinson
Copy link
Contributor Author

Hmm yeh prerelease to prerelease on a different version is a bit confusing. Maybe prerelease to prerelease should also always be pre* ?

@tjenkinson
Copy link
Contributor Author

Is the definition of pre* meant to be ‘going to a prerelease version’? If that’s the case those 3 tests should be pre*

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants