fix findBestMatch so it finds the best match and not the first match #6611
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks again for the review @Jarred-Sumner. I've made the changes requested. It's a lot cleaner now thanks to your feedback, and I understand even more! |
| return .{ .version = version, .package = &packages[i - 1] }; | ||
| // If we find one that matches, save it, but keep looping because we might find a newer match. | ||
| // The versions from the registry are not in any particular order. | ||
| if (group.satisfies(version) and (best_match_version == null or Semver.Version.order(version, best_match_version.?, "", "") == .gt)) { |
There was a problem hiding this comment.
Do we need to account for pre/post tags? Usually the way we do this is to get the string buffer from lockfile.buffers.string_bytes.items and pass that to the parent function
Suggested change
| if (group.satisfies(version) and (best_match_version == null or Semver.Version.order(version, best_match_version.?, "", "") == .gt)) { | |
| if (group.satisfies(version) and (best_match_version == null or Semver.Version.order(version, best_match_version.?, string_bytes, string_bytes) == .gt)) { |
|
I have some changes I'd like to make to this PR but it's a great start so I'm going to go ahead and merge it and then apply my changes afterwards. Thank you. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes
findBestMatch.Bug Explained
findBestMatchwas picking the first version that satisfies semver range, but registries don't return versions in any particular order.This was easily repeatable with
lines-and-columns@^1.1.6, it would always pick the first version1.2.0, which happens to be deprecated and broken, instead of the best match, which is1.2.4.This PR should help with a bunch of install-related bugs:
How did you verify your code works?
This modifies zig code
zig fmton the changed filesFirst time Zig user
I've contributed to Bun before, but this is my first time contributing to the zig code. I installed Zig and its dependencies this morning. I'm happy to accept input to improve this PR!