"Pad" shorter versions with empty parts when comparing #5001
+29
−28
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
microsoft-github-policy-service
bot
added
the
Issue-Bug
florelis
previously approved these changes
Nov 22, 2024
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
florelis
previously approved these changes
Nov 22, 2024
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
I'll get it right one of these times! |
florelis
previously approved these changes
Nov 22, 2024
Agreed and updated (and apologies for all the re-reviews @florelis!) |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
florelis
approved these changes
Nov 23, 2024
|
@florelis - Should I raise a cherry pick into 1.9, or has that next patch already been built? |
Go ahead and create a PR for the cherry-pick. I'm fine with bringing the change to 1.9, but I'll leave the decision to @denelon / @JohnMcPMS We haven't made a build with the latest changes, and most likely won't until after the holiday weekend. |
Trenly
added a commit
to Trenly/winget-cli
that referenced
this pull request
Nov 25, 2024
<!-- To check a checkbox place an "x" between the brackets. e.g: [x] --> - [x] I have signed the [Contributor License Agreement](https://cla.opensource.microsoft.com/microsoft/winget-pkgs). - [x] This pull request is related to an issue. - Resolves microsoft#4998 When versions are created, any empty parts are trimmed off. This means that `22.0.0` is stored as `[22]` in memory. This causes an issue when comparing to a version that had additional parts, but should be sorted lower than the version which is trimmed, such as `22.0.0-rc`. This is due to the comparator seeing that `[22]` has no more parts to compare after the first iteration, while `[22, 0, 0-rc]` does, causing it to hit the condition where whichever version has more parts is considered to be greater. This PR updates the logic so that the version comparison effectively contains as many empty parts as are needed to fully complete the comparison. This retains the logic that if a part is not equal to the part it is being compared against, the result of that comparison will be returned but eliminates the need for checking if the versions have the same number of parts since, effectively, they do; it's just that the extra parts needed to complete the comparison are all empty. Additional tests have been added to cover this corner case. If possible, I would ask that this also be cherry-picked into any future 1.9 releases (assuming this PR passes review and merges) ###### Microsoft Reviewers: [Open in CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/microsoft/winget-cli/pull/5001)
Trenly
added a commit
to Trenly/winget-cli
that referenced
this pull request
Nov 25, 2024
<!-- To check a checkbox place an "x" between the brackets. e.g: [x] --> - [x] I have signed the [Contributor License Agreement](https://cla.opensource.microsoft.com/microsoft/winget-pkgs). - [x] This pull request is related to an issue. - Resolves microsoft#4998 When versions are created, any empty parts are trimmed off. This means that `22.0.0` is stored as `[22]` in memory. This causes an issue when comparing to a version that had additional parts, but should be sorted lower than the version which is trimmed, such as `22.0.0-rc`. This is due to the comparator seeing that `[22]` has no more parts to compare after the first iteration, while `[22, 0, 0-rc]` does, causing it to hit the condition where whichever version has more parts is considered to be greater. This PR updates the logic so that the version comparison effectively contains as many empty parts as are needed to fully complete the comparison. This retains the logic that if a part is not equal to the part it is being compared against, the result of that comparison will be returned but eliminates the need for checking if the versions have the same number of parts since, effectively, they do; it's just that the extra parts needed to complete the comparison are all empty. Additional tests have been added to cover this corner case. If possible, I would ask that this also be cherry-picked into any future 1.9 releases (assuming this PR passes review and merges) ###### Microsoft Reviewers: [Open in CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/microsoft/winget-cli/pull/5001)
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.
When versions are created, any empty parts are trimmed off. This means that
22.0.0is stored as[22]in memory. This causes an issue when comparing to a version that had additional parts, but should be sorted lower than the version which is trimmed, such as22.0.0-rc. This is due to the comparator seeing that[22]has no more parts to compare after the first iteration, while[22, 0, 0-rc]does, causing it to hit the condition where whichever version has more parts is considered to be greater.This PR updates the logic so that the version comparison effectively contains as many empty parts as are needed to fully complete the comparison. This retains the logic that if a part is not equal to the part it is being compared against, the result of that comparison will be returned but eliminates the need for checking if the versions have the same number of parts since, effectively, they do; it's just that the extra parts needed to complete the comparison are all empty.
Additional tests have been added to cover this corner case.
If possible, I would ask that this also be cherry-picked into any future 1.9 releases (assuming this PR passes review and merges)
Microsoft Reviewers: Open in CodeFlow