-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Handle invalid versions gracefully for all list formats #13345
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
base: main
Are you sure you want to change the base?
Conversation
51d6da5
to
ac6533e
Compare
Annnnd changing the JSON format to emit the raw version broke our tests. That's enough of a signal to not mess with the format more than necessary. I've updated the PR to only use the raw version as a fallback. |
try: | ||
req_string = f"{dist.raw_name}=={dist.version}" | ||
except InvalidVersion: | ||
req_string = f"{dist.raw_name}==={dist.raw_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.
The pip list --format=freeze
does not produce identical output as pip freeze
. For instance direct URLs are not supported. So I'm not sure emitting a === will be what user expect here, but to be honest I don't know why this output format for pip list
exists in addition to pip freeze
.
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.
Ah, are you saying that since pip list --format=freeze
has never matched the freeze format that people may not expect anything but name==version
? That's fair, although I have doubts on how problematic ===
would be. A) previously, pip list would crash as it had no way of handling invalid versions, and B) the verbose mode already augments the pip list --format=freeze
format.
The alternative is that we simply hide the package with broken metadata or let pip continue crashing, and I don't think that's a particularly great outcome. We could warn that a package is hidden due to broken metadata, but given other parts of pip handle invalid versions gracefully, that would be also unexpected IMO.
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.
are you saying that since
pip list --format=freeze
has never matched the freeze format that people may not expect anything butname==version
?
Yes, that, but maybe I'm overthinking it.
I'm not sure if it's the right call to always emit the raw version in the JSON format, but given that the columns (default) format does emit the raw version, it seemed better to match that. The freeze format is unique because it's meant to be used for reinstallation so normalized versions are preferable. OTOH, JSON is also meant as a machine-readable format, so perhaps we should display the normalized version if possible.I left the normalization intact since changing it broke the tests.Resolves #13341.