-
Notifications
You must be signed in to change notification settings - Fork 250
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
Normalize specifier version for prefix matching #561
Conversation
608c00e
to
5e5932c
Compare
5e5932c
to
50a5fd6
Compare
50a5fd6
to
d7db7a9
Compare
packaging/specifiers.py
Outdated
# Convert our spec string into a Version ignoring the trailing .* | ||
# This allows to get the normalized version string | ||
spec_version = Version(spec[:-2]) | ||
# Split the spec out by dots, and pretend that there is an implicit | ||
# dot in between a release segment and a pre-release segment. | ||
split_spec = _version_split(spec[:-2]) # Remove the trailing .* | ||
split_spec = _version_split(str(spec_version)) |
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.
Does normalize_version
work here? It’d be much cheaper than constructing a Version object and converting it immediately back to str.
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.
Sorry for the late reply, I was away on vacation with limited time to answer and bad internet access.
Does normalize_version work here? It’d be much cheaper than constructing a Version object and converting it immediately back to str.
canonicalize_version
works with strip_trailing_zero=False
. However, I doubt it'll be cheaper given it does the same thing: construct a Version object and builds a string similar to Version.__str__
with added conditional statements on strip_trailing_zero
and version argument type check.
I can understand that it's clearer with canonicalize_version
and can go this way if clarity beats speed cost here.
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.
Clarity (almost) always beats speed. 🙂
@mayeut would you mind answering Tzu-Ping's question about |
3979529
to
c176ef4
Compare
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.
Let's go ahead and use canonicalize_version()
for clarity purposes (I would have made the suggestion in a review comment, but GitHub won't let me due to the supposed deleted line). Otherwise LGTM!
ab2ee78
to
2d38992
Compare
done |
2d38992
to
ea6f93a
Compare
In case of prefix version matching, normalization of the version was not applied to the specifier's version. This resulted in `2` being rejected as a candidate for `==0!2.*` With normalization applied, the candidate is now accepted.
ea6f93a
to
8db8a5b
Compare
Thanks so much! |
In case of prefix version matching, normalization of the version was not applied to the specifier's version. This resulted in `2` being rejected as a candidate for `==0!2.*` With normalization applied, the candidate is now accepted.
In case of prefix version matching, normalization of the version was not applied to the specifier's version.
This resulted in
2
being rejected as a candidate for==0!2.*
With normalization applied, the candidate is now accepted.
fixes #583