-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Conversation
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 |
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 |
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. |
On review, I didn't see any concerning usage. Also, GH copilot will write the commit message for you when you edit here. Fun. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
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
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