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(diff): fix prerelease to stable version diff logic #755

Merged
merged 2 commits into from
Jan 29, 2025

Conversation

eminberkayd
Copy link
Contributor

Refactored version diff logic to handle transitions from prerelease versions to stable versions correctly for major/minor/patch version bumps.

Closes #606

Refactored version diff logic to handle transitions from prerelease versions
to stable versions correctly for major/minor/patch version bumps.
@eminberkayd eminberkayd requested a review from a team as a code owner January 21, 2025 22:40
@wraithgar
Copy link
Member

Looking at what needed to be done to fix this, and seeing the code that was left, I had a thought that this wasn't complete and I was right.

~/D/n/n/b/main (fix/prerelease-diff-logic-#606|✔) $ node
Welcome to Node.js v22.13.1.
Type ".help" for more information.
> require('.').diff('1.1.1-pre', '2.1.1')
'minor'
> 

So, I think you've identified what's wrong here (order of operations) but we obviously still have holes.

@wraithgar
Copy link
Member

diff --git a/functions/diff.js b/functions/diff.js
index f011d99..58f9fde 100644
--- a/functions/diff.js
+++ b/functions/diff.js
@@ -27,8 +27,13 @@ const diff = (version1, version2) => {
       return 'major'
     }
 
-    // Otherwise it can be determined by checking the high version
+    // If the majors differ then it's major
+    if (highVersion.major !== lowVersion.major) {
+      return 'major'
+    }
 
+    // Otherwise it can be determined by checking the high version
+    //
     if (highVersion.minor) {
       // anything higher than a minor bump would result in the wrong version
       return 'minor'
> require('.').diff('1.1.1-pre', '2.1.1-pre')
'premajor'
> require('.').diff('1.1.1-pre', '2.1.1')
'major'
> 

Tests still pass with this, would need to add test cases for this new case.

@wraithgar wraithgar mentioned this pull request Jan 28, 2025
@eminberkayd
Copy link
Contributor Author

I have incorporated additional test cases and updated the PR accordingly. Kindly review it when you have time, and let me know if any modifications are required. Thank you!

@wraithgar
Copy link
Member

Oh yes this is much better.

@wraithgar wraithgar merged commit d588e37 into npm:main Jan 29, 2025
29 checks passed
wraithgar pushed a commit that referenced this pull request Jan 29, 2025
🤖 I have created a release *beep* *boop*
---


## [7.7.0](v7.6.3...v7.7.0)
(2025-01-29)
### Features
*
[`0864b3c`](0864b3c)
[#753](#753) add "release" inc
type (#753) (@mbtools)
### Bug Fixes
*
[`d588e37`](d588e37)
[#755](#755) diff: fix prerelease
to stable version diff logic (#755) (@eminberkayd, berkay.daglar)
*
[`8a34bde`](8a34bde)
[#754](#754) add identifier
validation to `inc()` (#754) (@mbtools)
### Documentation
*
[`67e5478`](67e5478)
[#756](#756) readme: added
missing period for consistency (#756) (@shaymolcho)
*
[`868d4bb`](868d4bb)
[#749](#749) clarify comment
about obsolete prefixes (#749) (@mbtools, @ljharb)
### Chores
*
[`145c554`](145c554)
[#741](#741) bump
@npmcli/eslint-config from 4.0.5 to 5.0.0 (@dependabot[bot])
*
[`753e02b`](753e02b)
[#747](#747) bump
@npmcli/template-oss from 4.23.3 to 4.23.4 (#747) (@dependabot[bot],
@npm-cli-bot)
*
[`0b812d5`](0b812d5)
[#744](#744) postinstall for
dependabot template-oss PR (@hashtagchris)

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@eminberkayd eminberkayd deleted the fix/prerelease-diff-logic-#606 branch January 29, 2025 20:35
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.

[bug] diff("1.7.2-1", "1.8.1") returns patch, not minor
2 participants