Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: semver.diff prerelease to release recognition #533

Merged
merged 1 commit into from
Apr 10, 2023
Merged

Conversation

wraithgar
Copy link
Member

@wraithgar wraithgar commented Apr 5, 2023

Cleanup of #334
Closes #333

Authored by @deefactorial

@wraithgar wraithgar requested a review from a team as a code owner April 5, 2023 23:32
@wraithgar wraithgar requested review from lukekarrys and removed request for a team April 5, 2023 23:32
functions/diff.js Outdated Show resolved Hide resolved
functions/diff.js Outdated Show resolved Hide resolved
@H4ad
Copy link
Contributor

H4ad commented Apr 7, 2023

In this line:

const v1 = parse(version1)
const v2 = parse(version2)

If we parse it first, we can speedup this from 300k op/s to ~470k to ~500k op/s.

Like this:

  const v1 = parse(version1)
  const v2 = parse(version2)

  if (eq(v1, v2)) {

Because eq also parse the version, so we parse it twice in this method without need.

Should I open another PR for this one?

EDIT: I opened a PR just to not mix the bugfix and the perf fix.

@H4ad H4ad mentioned this pull request Apr 7, 2023
@wraithgar wraithgar force-pushed the pull/334 branch 3 times, most recently from 6a4fd89 to cc279ff Compare April 7, 2023 01:18
Copy link
Contributor

@nlf nlf left a comment

Choose a reason for hiding this comment

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

this one was a bit confusing and is a little unintuitive, but it does make sense. for future readers here's what helped me wrap my head around it:

the versions 1.0.0, 1.1.0-pre and 1.1.0 would sort in that same order. the transition from 1.0.0 to 1.1.0-pre could be considered a pre-release, but it's also a minor and since the minor is the more impactful change that's what we report the diff as.

@wraithgar wraithgar merged commit 796cbe2 into main Apr 10, 2023
@wraithgar wraithgar deleted the pull/334 branch April 10, 2023 17:30
@github-actions github-actions bot mentioned this pull request Apr 10, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] semver.diff returns incorrect responses
4 participants