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

Make _check_dist_requires_python() parallel _check_link_requires_python(), and add more complete tests #6528

Merged
merged 2 commits into from
May 25, 2019

Conversation

cjerdonek
Copy link
Member

@cjerdonek cjerdonek commented May 24, 2019

This PR (1) moves check_dist_requires_python() to resolve.py where it used, (2) makes its implementation closely parallel index.py's _check_link_requires_python(), (3) adds an ignore_requires_python argument (like_check_link_requires_python()), and (4) tests it quite fully (including log messages, exception messages, and all branch logic). It also sets us up for passing py_version_info to the Resolver class, which will help with other issues.

This is a refactor in preparation for addressing issue #5369 ("pip download --python-version fails when downloaded package doesn't support the Python version running pip") and related issues.

@cjerdonek cjerdonek added C: download About fetching data from PyPI and other sources skip news Does not need a NEWS file entry (eg: trivial changes) type: refactor Refactoring code labels May 24, 2019
@cjerdonek cjerdonek changed the title Simplify and more fully test _check_dist_requires_python() Make _check_dist_requires_python() parallel _check_link_requires_python(), and add more complete tests May 24, 2019
@cjerdonek cjerdonek force-pushed the check-dist-requires-python branch 5 times, most recently from 69fb9bb to 12db11c Compare May 24, 2019 10:58
Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

check_dist_requires_python may be used in other locations too (like for checking the highest version of pip that supports a given version of Python once support is dropped; that's a feature request marked "good first issue"). I don't think it should be moved into resolve.py or suffixed with an _.

I do like the other changes in this PR.

@cjerdonek
Copy link
Member Author

check_dist_requires_python may be used in other locations too

Can that wait until the future change actually happens? Right now _check_dist_requires_python() is only being used in one place, and the log messages in the function are specific to the use inside resolve.py. We might find that _check_dist_requires_python() isn't appropriate for future uses. This parallels the situation for _check_link_requires_python(), where the log messages in that function are specific and tied to the use inside index.py.

@cjerdonek
Copy link
Member Author

As a compromise, can I move just _get_requires_python() to the other location? I can see that function being useful elsewhere because it doesn't have special logging logic, etc, and has a narrower purpose.

@pradyunsg
Copy link
Member

can I move just _get_requires_python() to the other location?

Sounds fair to me.

@cjerdonek
Copy link
Member Author

Okay, thanks, @pradyunsg. I've updated the PR.

@pradyunsg pradyunsg merged commit 77da032 into pypa:master May 25, 2019
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 24, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation C: download About fetching data from PyPI and other sources skip news Does not need a NEWS file entry (eg: trivial changes) type: refactor Refactoring code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants