-
Notifications
You must be signed in to change notification settings - Fork 492
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
Changes from all commits
65b34a8
891b7da
f8c38e9
2c37998
f584c08
89ae1fe
f3fd4db
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
['0.0.2-1', '0.0.2', 'patch'], | ||
['0.0.2-1', '0.0.3', 'patch'], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The difference is actually two
|
||
['0.0.2-1', '0.1.0', 'minor'], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
['0.0.2-1', '1.0.0', 'major'], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
['0.1.0-1', '0.1.0', 'minor'], | ||
['1.0.0-1', '1.0.0', 'major'], | ||
['0.0.0-1', '0.0.0', 'prerelease'], | ||
wraithgar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
['1.0.1-1', '1.0.1', 'patch'], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
['0.0.0-1', '0.0.0', 'major'], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
['1.0.0-1', '2.0.0', 'major'], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again this is two major
|
||
['1.0.0-1', '2.0.0-1', 'premajor'], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
['1.0.0-1', '1.1.0-1', 'preminor'], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
['1.0.0-1', '1.0.1-1', 'prepatch'], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
].forEach((v) => { | ||
const version1 = v[0] | ||
const version2 = v[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.
Related to the last part of the PR description, this would make more sense to me if this was all replaced with
return 'patch'
The way
npm version
behaves with major and minor bumps when the current version is a prerelease and parts are0
is a bit confusing to meSpecifically I don't understand why there is a 0 special case here and here
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.
special case for inc minor
The comments tell the story there:
A preminor version is a version whose patch is
0
and has a prerelease identifier. Bumping a minor from that is a matter of dropping the prerelease identifier.special case for inc major
Same as before, only we are looking at minor and patch. A premajor version is a version whose minor and patch are
0
and has a prerelease identifier. Bumping a major from that is a matter of dropping the prerelease identifier.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 guess it's a bit strange that you can go from
1.0.1-0
to1.1.0
withnpm version minor
but you can't go from1.0.0-0
to1.1.0
because the command works differently for that case.I still don't really get why 0 is special 🤷. Seems a bit inconsistent
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.
It does appear inconsistent but that is due to the nature of what
1.0.0-0
is. It's the prerelease for a semver major version. Dropping that prerelease is a major, there is no smaller increment you can make outside of increasing the prerelease identifier.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.
Right yes that makes sense. The thing that's confusing I think is that 0.1.0 to 1.0.0 is concretely major, but I'm not sure going from 1.0.0-0 to 1.0.0 could be classed as a major change, it's just going prerelease to stable. Maybe patch/minor/major doesn't really make sense as a thing when going prerelease to stable 🤷
I think the difference between 1.0.1-0 and 1.0.1 has the same weight
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.
It does make sense if the result from diff when we're going from prerelease to stable release is meant to represent the diff between the previous stable version and the new one 💡, but that's a step I wouldn't expect the diff to be doing internally