Empty version range results in empty condition set #17116
Conversation
|
|
There was a problem hiding this comment.
Many thanks for your contribution @jwidauer
I think I'd prefer just an assert that give an error message than allowing empty version ranges, which reads at least confusing and not clear what should be the expected behavior.
| ['*, include_prerelease', False, ["1.5.1"], ["1.5.1-pre1", "2.1-pre1"]], | ||
| ['*, include_prerelease', None, ["1.5.1", "1.5.1-pre1", "2.1-pre1"], []], | ||
| ['*, include_prerelease', True, ["1.5.1", "1.5.1-pre1", "2.1-pre1", "*"], []], | ||
| ['*, include_prerelease', False, ["1.5.1", "*"], ["1.5.1-pre1", "2.1-pre1"]], |
There was a problem hiding this comment.
It seems you are adding * as a version to check, but this is really an invalid version, so this shouldn't be a case at all.
There was a problem hiding this comment.
I'm happy to remove it again, but just out of curiosity, why would * be an invalid version to check here?
My rational behind adding it was, to check that an "any" range is still contained / valid when compared to another "any" version range.
There was a problem hiding this comment.
Because if you try to create a package with version = "*" it will fail with:
ERROR: Invalid package version '*'So the version "*" cannot exist at all. The values in ["1.5.1", "*"] are exact versions, not version ranges, to validate if they are inside the range.
There was a problem hiding this comment.
Ah, makes sense! I'll remove it! Thanks for the clarification!
| @@ -33,6 +33,7 @@ | |||
| # pre-releases | |||
| ['', [[[">=", "0.0.0-"]]], ["1.0"], ["1.0-pre.1"]], | |||
| ['*, include_prerelease=True', [[[">=", "0.0.0-"]]], ["1.0", "1.0-pre.1", "0.0.0", "0.0.0-pre.1"], []], | |||
| [', include_prerelease=True', [[[">=", "0.0.0-"]]], ["1.0", "1.0-pre.1", "0.0.0", "0.0.0-pre.1"], []], | |||
There was a problem hiding this comment.
I am not sure we want to support this syntax at all, requires = "dep/[,include_prerelease=True" looks really ugly and not intuitive what would be the expected behavior.
There was a problem hiding this comment.
I do agree, but wasn't sure whether adding an exception for this would be the preferred option. My assumption would be to only raise an exception when the version range specifier is empty and include_prerelease is specified. Leaving "dep/[]" as a valid range, but disallowing "dep/[,include_prerelease]". Is this assumption correct?
There was a problem hiding this comment.
I see your point.
In retrospective, probably we shouldn't have allowed the empty [ ] in first place, it is possible that it slipped from previous implementations, or earlier unit tests. But you are right now that it is allowed, and we most likely don't want to risk breaking existing users that would rely on it, that it would be more consistent to allow the prereleases to work with the same syntax even if ugly.
So looks good, lets go for it.
Please check that you need to sign the CLA in order to be able to merge the PR. Many thanks for your contribution.
There was a problem hiding this comment.
Sounds good to me! I should have signed the CLA now and will push the discussed update to the test now.
jwidauer
force-pushed
the
fix-version-range-comparison
branch
from
ac46a9b to
9438bfd
Compare
Changelog: Bugfix: Empty version range results in empty condition set.
Docs: Omit
This PR fixes a bug that occurs when an empty version range is supplied.
Previously an empty version range would result in a
_ConditionSet, with an empty list ofconditions.This can lead to issues, when comparing two version ranges created from an empty version string. Meaning
VersionRange('').intersection(VersionRange('')) == None, which is wrong.This PR fixes this behavior and makes sure this is also properly checked for in the tests.