Skip to content

Don't rely on parameter order when creating VersionRange #5213

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

Merged
merged 5 commits into from
Feb 19, 2025

Conversation

Trenly
Copy link
Contributor

@Trenly Trenly commented Feb 16, 2025

I don't know whether or not this will resolve the above issue, but I also don't think it can hurt?

Instead of relying on the parameters to be in the correct order for minimum and maximum when creating a VersionRange, the code can instead just put them in the right order.


Microsoft Reviewers: Open in CodeFlow

@Trenly Trenly requested a review from a team as a code owner February 16, 2025 20:58
@JohnMcPMS
Copy link
Member

The thing I worry about with this is code that would have failed previously will now get through and may continue on with a flawed view of the range.

@Trenly
Copy link
Contributor Author

Trenly commented Feb 18, 2025

The thing I worry about with this is code that would have failed previously will now get through and may continue on with a flawed view of the range.

That was my concern as well, but I wondered if it truly mattered? Consumers of the range should be checking if a version falls within the range, or using other methods of the VersionRange class. So, in theory, as long as the class maps everything properly, it should be transparent to the consumer.

However, theory really only goes so far, so conpletely understand if you'd prefer to close this

I'd be interested to know how the values in the index were flip-flopped between the max and min

@JohnMcPMS
Copy link
Member

I'd be interested to know how the values in the index were flip-flopped between the max and min

That is the real root issue (other than our continued discussions on the whether version ranges make any sense at all 😄).

@yao-msft
Copy link
Contributor

yao-msft commented Feb 19, 2025

I'd be interested to know how the values in the index were flip-flopped between the max and min

That is the real root issue (other than our continued discussions on the whether version ranges make any sense at all 😄).

That's probably due to this change #5001 where the version comparison was fixed to correctly compare versions. It's likely the index was created with an old wingetutil and the new client has fixed code. I think a rebuild of the index (with latest wingetutil) will fix the index.

@Trenly
Copy link
Contributor Author

Trenly commented Feb 19, 2025

I'd be interested to know how the values in the index were flip-flopped between the max and min

That is the real root issue (other than our continued discussions on the whether version ranges make any sense at all 😄).

That's probably due to this change #5001 where the version comparison was fixed to correctly compare versions. It's likely the index was created with an old wingetutil and the new client has fixed code. I think a rebuild of the index (with latest wingetutil) will fix the index.

Ah, that makes sense; The old code would have parsed it as 6.1 instead of 6.1.0, and in that case would have returned the incorrect version comparison

@JohnMcPMS
Copy link
Member

Given that this is the root cause, I'm inclined to say that the best resolution likely is to accept a change along these lines. If we do rebuild, or in fact if a similar situation arises after this change makes it to the service, then <1.10 clients will fail similarly. The good thing is that while I am concerned about the potential for some code to make a mistake that is harder to diagnose after this change, we can simply review current usage to see if that is happening.

@JohnMcPMS
Copy link
Member

On review, I didn't see any concerning usage.

Also, GH copilot will write the commit message for you when you edit here. Fun.

@JohnMcPMS
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JohnMcPMS JohnMcPMS merged commit af81f1d into microsoft:master Feb 19, 2025
9 checks passed
@Trenly Trenly deleted the VersionRange branch February 19, 2025 21:32
JohnMcPMS pushed a commit to JohnMcPMS/winget-cli that referenced this pull request Feb 19, 2025
)

Instead of relying on the parameters to be in the correct order for
minimum and maximum when creating a VersionRange, the code can instead
just put them in the right order.
JohnMcPMS added a commit that referenced this pull request Feb 19, 2025
Instead of relying on the parameters to be in the correct order for
minimum and maximum when creating a VersionRange, the code can instead
just put them in the right order.

CP from #5213
Trenly added a commit to Trenly/winget-cli that referenced this pull request Feb 19, 2025
)

Instead of relying on the parameters to be in the correct order for
minimum and maximum when creating a VersionRange, the code can instead
just put them in the right order.
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.

3 participants