Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix PyPiVersionRange working with * and ~= #126

Closed
wants to merge 1 commit into from
Closed

Fix PyPiVersionRange working with * and ~= #126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 33 additions & 11 deletions src/univers/version_range.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -664,11 +664,6 @@ class PypiVersionRange(VersionRange):
">=": ">=",
"<": "<",
">": ">",
# per https://www.python.org/dev/peps/pep-0440/#compatible-release
# For a given release identifier V.N, the compatible release clause is
# approximately equivalent to the pair of comparison clauses:
# >= V.N, == V.*
"~=": None,
# 01.01.01 is NOT equal to 1.1.1 using === which is strict string
# equality this is a rare and eventually non-suggested approach
"===": None,
Expand All @@ -681,7 +676,7 @@ def from_native(cls, string):
Raise an a univers.versions.InvalidVersion
"""
# TODO: environment markers are yet supported
# TODO: handle .* version, ~= and === operators
# TODO: handle === operators

if ";" in string:
raise InvalidVersionRange(f"Unsupported PyPI environment marker: {string!r}")
Expand All @@ -694,6 +689,8 @@ def from_native(cls, string):
f"Unsupported character: {unsupported_chars!r} " f"in PyPI version: {string!r}"
)

string = cls.sanitize_constraints(string)

try:
specifiers = SpecifierSet(string)
except InvalidSpecifier as e:
Expand All @@ -707,14 +704,10 @@ def from_native(cls, string):
operator = spec.operator
version = spec.version

if operator == "~=" or operator == "===":
if operator == "===":
msg = f"Unsupported PyPI version constraint operator: {spec!r}"
unsupported_messages.append(msg)

if str(version).endswith(".*"):
msg = f"Unsupported PyPI version: {spec!r}"
unsupported_messages.append(msg)

try:
version = cls.version_class(version)
comparator = cls.vers_by_native_comparators[operator]
Expand All @@ -729,6 +722,35 @@ def from_native(cls, string):

return cls(constraints=constraints)

@classmethod
def sanitize_constraints(cls, string):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add tests specifically for this function? And some doc string?

constraints = []
try:
specifiers = SpecifierSet(string)
except InvalidSpecifier as e:
raise InvalidVersionRange() from e
for spec in specifiers:
operator = spec.operator
version = spec.version
if "*" in version:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we limit this to trailing "*" and not at any position?
I am not sure we can handle sanely other cases for now and the case with a star not at the end should likely be an exception.

Suggested change
if "*" in version:
if version.endswith("*"):

pos = version.find("*")
version = version[: pos - 1]
if "==" in operator:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using a strict equality? otherwise this is true: "==" in "==="

Suggested change
if "==" in operator:
if operator == "==":

The same applies to the tests below

constraints.extend(
[f">= {version}", f"< {version[:pos - 2]}{str(int(version[pos - 2]) + 1)}"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why keep a space between the operator and the version? Also pos - 2 is hard to read for me and not obvious... could there be a better way? may be with some intermediate variables to give it some meaning? And multiple append rather than an extend?

)
elif "!=" in operator:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
elif "!=" in operator:
elif operator == "!=":

constraints.extend(
[f"< {version}", f">= {version[:pos - 2]}{str(int(version[pos - 2]) + 1)}"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same nit as above: there must be a more readable way to express this.

)
elif "~=" in operator:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
elif "~=" in operator:
elif operator == "~=":

parts = version.split(".")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if there is no dot in the version? Will the code below crash?

parts[-2] = str(int(parts[-2]) + 1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a more obvious way to write this? This is really hard for me to understand :]

constraints.extend([f">= {version}", f"< {'.'.join(parts[:-1])}"])
else:
constraints.append(f"{operator} {version}")
return ", ".join(constraints)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the benefit of just sanitizing constraints and returning a string, vs. actually integrating this in the parsing of constraints proper? Here the code ends up parsing the same constraints set twice:



class MavenVersionRange(VersionRange):
"""
Expand Down
10 changes: 6 additions & 4 deletions tests/test_version_range.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,13 +357,14 @@ def test_from_native_and_from_string_round_trip(scheme, native_ranges):
@pytest.mark.parametrize(
"range, will_pass, expected",
[
(" ~= 0.9", False, "Unsupported PyPI version constraint operator"),
("~= 1.3", False, "Unsupported PyPI version constraint operator"),
(" ~= 0.9", True, "vers:pypi/>=0.9|<1"),
("~= 1.3", True, "vers:pypi/>=1.3|<2"),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about these added tests:

"==1*"
"~=1"
"~=1*"
"!=1*"
">=1*"
">1*"
"<=1*"
"<1*"
"~=1.*.*"
"~=*.1"
"~=1.*.2"
"~=*"

(" >= 1.0", True, "vers:pypi/>=1.0"),
(" != 1.3.4.*", False, "Unsupported PyPI version"),
(" != 1.3.4.*", True, "vers:pypi/<1.3.4|>=1.3.5"),
("< 2.0", True, "vers:pypi/<2.0"),
("~= 1.3.4.*", False, ""),
("==1. *", False, "Unsupported PyPI version"),
("==1. *", True, "vers:pypi/>=1|<2"),
("!=1.2.*", True, "vers:pypi/<1.2|>=1.3"),
("==1.3.4 ) (", False, "Unsupported character"),
("===1.0", False, "Unsupported PyPI version"),
],
Expand All @@ -375,6 +376,7 @@ def test_PypiVersionRange_raises_ivr_for_unsupported_and_invalid_ranges(range, w
raise Exception("Exception not raised")
except InvalidVersionRange as ivre:
assert expected in str(ivre)

else:
assert expected == str(PypiVersionRange.from_native(range))

Expand Down