-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Sort Requires
and Required-By
fields for pip show
#10422
Conversation
src/pip/_internal/commands/show.py
Outdated
requires = sorted( | ||
(req.name for req in dist.iter_dependencies()), key=lambda pkg: pkg.lower() | ||
) | ||
required_by = sorted(_get_requiring_packages(dist), key=lambda pkg: pkg.lower()) |
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.
Use operator.methodcaller("lower")
instead.
Also, since _get_requiring_packages
not longer needs return a list, please change it to return an iterator instead.
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.
We could even use key=str.lower
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.
@uranusjr, good point. I have converted it to an iterator.
@pradyunsg, I didn't think of that. It's definitely nicer than a lambda. I have changed it.
On a side note, I don't really know what the development process is here. Should I squash and rebase once this is approved and before being merged, or is that not something I need to worry about?
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.
On second thought, this probably deserves a line in the change log. Could you change the news file to use feature
and add a one-line description? Something like
Out of ``pip show`` now sorts ``Requires`` and ``Required-By`` alphabetically.
@uranusjr, makes sense. I've deleted the old news file and created a feature one with similar wording. |
Thanks @aphedges! ^>^ |
I have noticed that when running
pip show
,Requires
has a non-deterministic order across runs andRequired-By
has deterministic order but without any clear logic behind the order. I propose that we change this to improve readability. This was first a problem for me when it made comparing the results ofpip show
across environments more difficult than a simple diff.Of the three other
List
fields, one (Files
) is already being sorted, so this change does not seem out-of-place. The remaining other fields (Classfiers
andEntry-points
) are not sorted inpip show
, but they might be sorted elsewhere or are intentionally left unsorted, so I did not touch them.I sort the packages case-insensitively to match the logic in
pip list
andpip freeze
.This seems like a
trivial
change to me, but if you think this requires further discussion, I can make an issue for it.