Using packaging.version instead of pkg_resources #2825
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files☔ View full report in Codecov by Sentry. |
| version_parser(v) | ||
| for v in desired_revisions | ||
| if re.match(r"\d+\.\d+(?:\.\d+)*(?:[\w\-_])*", v) | ||
| str(Version(v)) for v in desired_revisions if re.match(r"\d+\.\d+(?:\.\d+)*(?:[\w\-_])*", v) |
There was a problem hiding this comment.
The problem with the usage of packaging.Version() as it stands here now is mentioned in the comment above.
desired revisions may contain arbitrary branch names that do not correspond to valid semantic versioning patterns.
And packaging.Version has its own definition of what a valid version identifier is and it is not completely compatible with semantic versions either.
As an example:
>>> [str(Version(v)) for v in ["1.1.1-latest"] if re.match(r"\d+\.\d+(?:\.\d+)*(?:[\w\-_])*", v)]
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "<stdin>", line 1, in <listcomp>
File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/packaging/version.py", line 298, in __init__
raise InvalidVersion("Invalid version: '{0}'".format(version))
packaging.version.InvalidVersion: Invalid version: '1.1.1-latest'
This will not work nicely if any of the desired revisions fails validity checking inside Version().
The previous version parser from pkg_resources import parse_version returns a LegacyVersion if it can't parse the identifier, which makes it much better suited in this context than packaging.Version.
>>> [version_parser(v) for v in ["1.1.1", "1.2.3-latest"]]
[<Version('1.1.1')>, <LegacyVersion('1.2.3-latest')>]
I am not versed enough to know what kind of strings can appear in desired_revisions but I believe this code should be able to sort a broader range of version specifiers.
| version_parser(v) | ||
| for v in desired_revisions | ||
| if re.match(r"\d+\.\d+(?:\.\d+)*(?:[\w\-_])*", v) | ||
| str(Version(v)) for v in desired_revisions if re.match(r"\d+\.\d+(?:\.\d+)*(?:[\w\-_])*", v) |
There was a problem hiding this comment.
A separate issue is the conversion of the Version instance to a string. It will lead to wrong orderings as demonstrated below with a version number which had a release candidate.
>>> sorted(str(Version(v)) for v in ['1.23.4', '1.23.4-1', '1.23.5', '1.23.5rc'])[-1]
'1.23.5rc0'
>>> sorted(Version(v) for v in ['1.23.4', '1.23.4-1', '1.23.5', '1.23.5rc'])[-1]
<Version('1.23.5')>
There was a problem hiding this comment.
I think as the PR stands now with the use of packaging.Version has a good chance of breaking some use cases because it sets tighter restrictions on what a valid revision is and Throws an exception if this stricter requirement isn't met.
The conversion of the Version objects to strings as it currently stands will lead to erroneous sorting of version identifiers and should be dropped.
#1984
PR checklist
CHANGELOG.mdis updateddocsis updated