Fix PyPiVersionRange working with * and ~= - #126
Conversation
Signed-off-by: GermanMT <germanoctako@gmail.com>
pombredanne
left a comment
pombredanne
left a comment
There was a problem hiding this comment.
Thank you ++! See some comments for your consideration.
| if "*" in version: | ||
| pos = version.find("*") | ||
| version = version[: pos - 1] | ||
| if "==" in operator: |
There was a problem hiding this comment.
What about using a strict equality? otherwise this is true: "==" in "==="
| if "==" in operator: | |
| if operator == "==": |
The same applies to the tests below
| for spec in specifiers: | ||
| operator = spec.operator | ||
| version = spec.version | ||
| if "*" in version: |
There was a problem hiding this comment.
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.
| if "*" in version: | |
| if version.endswith("*"): |
| version = version[: pos - 1] | ||
| if "==" in operator: | ||
| constraints.extend( | ||
| [f">= {version}", f"< {version[:pos - 2]}{str(int(version[pos - 2]) + 1)}"] |
There was a problem hiding this comment.
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?
| constraints.extend( | ||
| [f">= {version}", f"< {version[:pos - 2]}{str(int(version[pos - 2]) + 1)}"] | ||
| ) | ||
| elif "!=" in operator: |
There was a problem hiding this comment.
| elif "!=" in operator: | |
| elif operator == "!=": |
| ) | ||
| elif "!=" in operator: | ||
| constraints.extend( | ||
| [f"< {version}", f">= {version[:pos - 2]}{str(int(version[pos - 2]) + 1)}"] |
There was a problem hiding this comment.
Same nit as above: there must be a more readable way to express this.
| [f"< {version}", f">= {version[:pos - 2]}{str(int(version[pos - 2]) + 1)}"] | ||
| ) | ||
| elif "~=" in operator: | ||
| parts = version.split(".") |
There was a problem hiding this comment.
What if there is no dot in the version? Will the code below crash?
| ) | ||
| elif "~=" in operator: | ||
| parts = version.split(".") | ||
| parts[-2] = str(int(parts[-2]) + 1) |
There was a problem hiding this comment.
Is there a more obvious way to write this? This is really hard for me to understand :]
| return cls(constraints=constraints) | ||
|
|
||
| @classmethod | ||
| def sanitize_constraints(cls, string): |
There was a problem hiding this comment.
Could you add tests specifically for this function? And some doc string?
| (" ~= 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"), |
There was a problem hiding this comment.
What about these added tests:
"==1*"
"~=1"
"~=1*"
"!=1*"
">=1*"
">1*"
"<=1*"
"<1*"
"~=1.*.*"
"~=*.1"
"~=1.*.2"
"~=*"
| constraints.extend([f">= {version}", f"< {'.'.join(parts[:-1])}"]) | ||
| else: | ||
| constraints.append(f"{operator} {version}") | ||
| return ", ".join(constraints) |
There was a problem hiding this comment.
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:
|
BTW, do you have URLs to some concrete examples of where these constraints are used in the wild? Thanks! |
|
@GermanMT too bad you closed this! |
2 participants
Fix the problem with the ~= operator and the versions ending in *, exposed in the issue.
Fixes:
PyPiVersionRange.from_nativenot working #26