-
Notifications
You must be signed in to change notification settings - Fork 278
Relax typing of _key on _BaseVersion
#669
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
Conversation
|
Would PyPI's model be easier if packaging just kept the LegacyVersion class and didn't use it; such that PyPI had to keep its own parse_version that does what the old one did? |
|
I'm asking since it'll be cleaner to have the implementation-used-by-pypi be available in this package rather than somewhere else. What we want is to push all packages used with build/run tooling, regardless of whether they're on PyPI or not, to use PEP 440 style versions. |
|
I think it's fine for legacy version numbers to require extra opt in somehow... maybe that's the One thing I'm curious about is whether Warehouse actually needs this. I haven't looking really closely, but for instance you should be able to implement version ordering by doing something like: from packaging.version import Version
def _version_key(version):
try:
return (1, Version(version))
except Exception: # I forget the real exception offhand
return (0, version)
versions = ["1.0-dog", "1.0"]
versions.sort(key=_version_key)I don't recall if we actually pull information out of version objects or not, or if it's primarily just sorting. |
It doesn't really make any difference to me. I think it's probably easier for everyone to just maintain this separately.
It's not just sorting, we do attempt to parse the legacy versions in a few places as well, though we could maybe avoid that for legacy versions. I can explore it. Here's the diff I have now: |
|
(One example is on the project detail page, when we compare a given release's version to the latest version: https://github.com/pypi/warehouse/blob/aa56a2a50d9578ffb1886ce48d0773ad5853e036/warehouse/templates/packaging/detail.html#L176-L183) |
|
To be clear, I don't have a strong preference. I was mostly just an idle thought if that would end up being an easy solution or not. |
|
Is If that is the case and |
After poking it a bit, I don't think it's going to be easy given that we still need to parse/compare legacy versions. I think we'd just end up re-implementing
I don't think so. Given that we've deprecated/dropped legacy versions here, I would expect any metadata with a legacy version to be considered invalid by this library.
Given how long they've been deprecated here and how long they've been invalid on PyPI, it's probably fine for downstream tools to just consider them invalid too. I think anyone attempting to do something with metadata with legacy versions is doing something sufficiently weird/niche (like, being PyPI) that mainstream/modern tooling doesn't need to accommodate it. |
Specifically, the direction |
|
FWIW, I need to parse eventually any Python version in the various tools I maintain for dependency resolution and vulnerable version range processing. @pradyunsg kindly warned long before dropping LegacyVersion but this was still a source of intense churn in December when packaging 23 was released. I then created a temp fork of packaging called https://github.com/nexB/packvers/ (which is now used in several tools as a stop gap) and experimented with vendoring just the legacy version code in https://github.com/nexB/pip-requirements-parser/blob/main/src/packaging_legacy_version.py . pip-requirements-parser is used in pip-audit and a few other places and is reported as being "critical" by PyPI. .... BUT none of these solutions (packvers or vendoring) is really satisfying nor working correctly wrt. subclassing and interop with packaging. I'd like to help in anyway I can and it would be awesome to get something clean I can migrate to! |
|
@pombredanne Thanks for chiming in. Seems like this and https://github.com/di/packaging_legacy should suit your needs. @pypa/packaging-committers IMO, we should move forward with this given the discussion here. |
|
I'm good with accepting this if @pradyunsg is. |
|
@pradyunsg Friendly bump here. I'd like to get PyPI onto the latest version of packaging sooner than later. |
TL;DR:
Changing this typing allows other libraries to extend
_BaseVersionand introduce new subclasses that are comparable withVersion, and allows static typing to remain correct for bothpackagingand these other libraries.Long story:
PyPI is currently unable to upgrade to the latest version of
packagingbecause we need to continue usingLegacyVersions -- we still host releases with "legacy" versions, still need to sort "legacy" versions along with "non-legacy" versions, etc. and will likely continue doing so indefinitely.Instead of remaining stuck on an old version of
packagingforever, I'm breaking out the removedLegacyVersionclass into a new package,packaging_legacythat provides a drop-in replacement with the removed functionality, deferring as much as possible to the upstreampackaginglibrary, and evolving in sync with new releases here.(Right now, this only resurrects
packaging.version.LegacyVersionandpackaging.version.parse) because that's all we need.)Overall, this works great, however type checking fails because
_BaseVersion._keyis typed too strictly. The old annotation wasUnion[CmpKey, LegacyCmpKey], and the current annotationCmpKeydoesn't allow me to define aLegacyVersionclass that inherits from_BaseVersionand provides a_keywith the typeLegacyCmpKey.To resolve this, this PR relaxes the typing of
_BaseVersion._keytoTuple[Any, ...]and then moves the stricterCmpKeytype toVersion._key. This should have zero effect on this library and make no difference to anyone downstream.PS: I'll acknowledge the overall idea is somewhat gross, but I think it's the best way to support things that need to continue supporting legacy versions (as was raised in #530 and #631) without introducing a complete fork of this library or vendoring
LegacyVersionand everything related to it into things that need to use it. If you have better ideas, I'd love to hear them.