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

Search all distributions when trying to find a match in get_distribution() #8702

Merged
merged 5 commits into from
Aug 10, 2020

Conversation

uranusjr
Copy link
Member

@uranusjr uranusjr commented Aug 4, 2020

Fix #8695. I also made some minor refactoring so we can better avoid this kind of behavioural mistakes in the future.

This matches the behavior of pkg_resources.get_distribution(), which
this function intends to replace.
The call to get_installed_distributions() now passes all flags
excplicitly so they are more obvious and less likely to be misunderstood
in the future. The behavior also documented in the function docstring.

The search_distribution() helper function is renamed with a leading
underscore to make it clear that it is intended as a helper function to
get_distribution().
@@ -140,6 +140,7 @@ def print_results(hits, name_column_width=None, terminal_width=None):
write_output(line)
if name in installed_packages:
dist = get_distribution(name)
assert dist is not None
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole module is using the distribution-related functions in a very inprecise way. I’ll submit a standalone refactoring PR after this is accepted

Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks! The two other call sites of misc.get_distribution should not be affected by this change.

I'm not sure how we could add a test for this. Perhaps by mocking dist_is_local?

@sbidoul sbidoul added this to the 20.2.2 milestone Aug 5, 2020
@sbidoul
Copy link
Member

sbidoul commented Aug 5, 2020

@pradyunsg I took the liberty to create milestone 20.2.2

@uranusjr
Copy link
Member Author

uranusjr commented Aug 5, 2020

I'm not sure how we could add a test for this. Perhaps by mocking dist_is_local?

Me either, non-local site-packages are difficult to test, for obvious reasons. I’ll take a look later and see whether existing tests can offer some inspiration.

@uranusjr
Copy link
Member Author

uranusjr commented Aug 6, 2020

I added a couple of tests for this.

@jessecooper
Copy link

When is 20.2.2 going to be released? A regression in pip is really impactful to the community and can not be left hanging for days on end.

Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests look good to me, thanks for adding them @uranusjr.

@uranusjr
Copy link
Member Author

When is 20.2.2 going to be released?

See #8511 (comment).

@pradyunsg pradyunsg merged commit 4c7bbdb into pypa:master Aug 10, 2020
pradyunsg added a commit to pradyunsg/pip that referenced this pull request Aug 11, 2020
@uranusjr uranusjr deleted the get-distribution-looks-for-all branch September 28, 2020 14:28
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 9, 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.

pip>=20.2 needlessly re-installs dependencies available on the system in system-site-packages venv
4 participants