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

Fix download package regardless of running python version #6483

Closed
wants to merge 0 commits into from
Closed

Fix download package regardless of running python version #6483

wants to merge 0 commits into from

Conversation

ppiyakk2
Copy link
Contributor

@ppiyakk2 ppiyakk2 commented May 8, 2019

fixed: #5369

When the package described only support over python 3.5 and the python version of running pip is python 2.7, there will be occure an error during download package.

To fix this problem, I added an additional option 'ignore_requires_python' to ignore the requires python version when checking the support python version of package.

@cjerdonek
Copy link
Member

@auvipy I think PR’s with behavior changes should have tests before they are marked approve.

In this case, I think a unit test should be included among the tests as CandidateEvaluator can now be tested easily in isolation.

@ppiyakk2
Copy link
Contributor Author

ppiyakk2 commented May 9, 2019

@cjerdonek Thanks for your comment. I agreed with the this PR should have tests. But Actually I'm not good at writing test case, Could you give me some idea about test case?

check_requires_python() function is using sys.version_info to get a running python version.

I want to write testcase like below

  1. If self._ignore_requires_python is False and required python version of package is 3.4
    -> then, If the running python version is python 2.7, the package will not be a candidate.
    -> else, the running python version is python 3.5, the package will be a candidate.

  2. If self._ignore_requires_python is True, the package always be a candidate.

So, I think the test case can be affected by running python version, Is there any good idea to solve this problem?

@auvipy
Copy link

auvipy commented May 9, 2019

@auvipy I think PR’s with behavior changes should have tests before they are marked approve.

In this case, I think a unit test should be included among the tests as CandidateEvaluator can now be tested easily in isolation.

I do agree. I somehow missed that this PR doesn't include tests!! will be more careful!

@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label May 13, 2019
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label May 14, 2019
@ppiyakk2 ppiyakk2 closed this May 14, 2019
@ppiyakk2 ppiyakk2 deleted the bugfix/5369 branch May 14, 2019 11:16
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 13, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 13, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pip download --python-version fails when downloaded package doesn't support the Python version running pip
5 participants