Skip to content
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

Merged
merged 4 commits into from
Sep 21, 2021

Conversation

aphedges
Copy link
Contributor

@aphedges aphedges commented Sep 2, 2021

I have noticed that when running pip show, Requires has a non-deterministic order across runs and Required-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 of pip 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 and Entry-points) are not sorted in pip 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 and pip freeze.

This seems like a trivial change to me, but if you think this requires further discussion, I can make an issue for it.

Comment on lines 158 to 161
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())
Copy link
Member

@uranusjr uranusjr Sep 3, 2021

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.

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

@uranusjr uranusjr left a 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.

@aphedges
Copy link
Contributor Author

aphedges commented Sep 4, 2021

@uranusjr, makes sense. I've deleted the old news file and created a feature one with similar wording.

@uranusjr uranusjr added this to the 21.3 milestone Sep 5, 2021
@pradyunsg pradyunsg merged commit 0c86e3c into pypa:main Sep 21, 2021
@pradyunsg
Copy link
Member

Thanks @aphedges! ^>^

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants