-
Notifications
You must be signed in to change notification settings - Fork 249
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
markers: Consistently fallback to Python comparison rules #816
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,6 @@ | |
default_environment, | ||
format_full_version, | ||
) | ||
from packaging.version import InvalidVersion | ||
|
||
VARIABLES = [ | ||
"extra", | ||
|
@@ -388,12 +387,39 @@ def test_extra_str_normalization(self): | |
assert str(Marker(rhs)) == f'extra == "{normalized_name}"' | ||
|
||
def test_python_full_version_untagged_user_provided(self): | ||
"""A user-provided python_full_version ending with a + fails to parse.""" | ||
with pytest.raises(InvalidVersion): | ||
Marker("python_full_version < '3.12'").evaluate( | ||
{"python_full_version": "3.11.1+"} | ||
) | ||
"""A user-provided python_full_version ending with a + uses Python behaviour.""" | ||
env = {"python_full_version": "3.11.1+"} | ||
assert Marker("python_full_version < '3.12'").evaluate(env) | ||
|
||
def test_python_full_version_untagged(self): | ||
with mock.patch("platform.python_version", return_value="3.11.1+"): | ||
assert Marker("python_full_version < '3.12'").evaluate() | ||
|
||
@pytest.mark.parametrize( | ||
("marker_string", "environment", "expected"), | ||
[ | ||
("platform_release >= '20.0'", {"platform_release": "21-foobar"}, True), | ||
("platform_release >= '8'", {"platform_release": "6.7.0-gentoo"}, False), | ||
("platform_version == '27'", {"platform_version": "weird string"}, False), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is very much a personal opinion, so feel free to ignore it, but it might be nice to have at least one test case that shows string comparison giving the opposite result from how a human would do a version comparison. Like the one you put in the PR description:
I'm imagining a different algorithm that extracts the longest prefix of each string which constitutes a valid PEP 440 version number and then compares based on that, which I think would pass all the existing test cases, but of course that is not the algorithm PEP 508 specifies so IMO it'd be nice to have a test case that would catch that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree -- this would be good to add here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did do that in |
||
# This looks weird, but is expected as per Python's lexicographical order. | ||
("platform_version >= '10'", {"platform_version": "6.7.0-gentoo"}, True), | ||
( | ||
"implementation_version == '3.*'", | ||
{"implementation_version": "2_private"}, | ||
False, | ||
), | ||
( | ||
"implementation_version == '3.*'", | ||
{"implementation_version": "3.*"}, | ||
True, | ||
), | ||
], | ||
) | ||
def test_valid_specifier_invalid_version_fallback_to_python( | ||
self, marker_string: str, environment: dict, expected: bool | ||
): | ||
"""If the right operand is a valid version specifier, but the | ||
left operand is not a valid version, fallback to Python string | ||
comparison behaviour. | ||
""" | ||
assert Marker(marker_string).evaluate(environment) == expected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this change, this fallbacks to Python comparison semantics so it technically works now.
cc @sbidoul for visibility that you're okay with this change.