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.

Sign up for GitHub

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

Jump to bottom

Make PIP_NO_CACHE_DIR behave as it reads, and not crash pip #5884

Merged
merged 1 commit into from
Nov 8, 2018
Merged

Make PIP_NO_CACHE_DIR behave as it reads, and not crash pip #5884

merged 1 commit into from
Nov 8, 2018

Conversation

cjerdonek
Copy link
Member

@cjerdonek cjerdonek commented Oct 14, 2018 •
edited
Loading

This fixes issues #5385 and #5735 (the PIP_NO_CACHE_DIR half).

This solution preserves backwards compatibility by only changing the behavior for values like "1", "true", "yes", etc, which didn't work before -- only crashing pip.

@cjerdonek cjerdonek force-pushed the pip-no-cache-dir-var branch 3 times, most recently from 2359f89 to 66a56a3 Compare October 14, 2018 08:43
This was referenced Oct 14, 2018
Closed
Open
@cjerdonek cjerdonek added C: configuration Configuration management and loading C: cache Dealing with cache and files in it T: bugfix labels Oct 14, 2018
@cjerdonek cjerdonek force-pushed the pip-no-cache-dir-var branch 3 times, most recently from 99078fa to 39e80c8 Compare October 14, 2018 17:08
@cjerdonek cjerdonek added this to the 19.0 milestone Oct 14, 2018
@cjerdonek cjerdonek force-pushed the pip-no-cache-dir-var branch from 39e80c8 to cb24ca6 Compare October 15, 2018 03:17
@cjerdonek cjerdonek mentioned this pull request Oct 16, 2018
Merged
8 tasks
pradyunsg
pradyunsg reviewed Oct 22, 2018
View reviewed changes
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.

I like the tests! :)

Inline suggestion about changing on the implementation approach.

src/pip/_internal/cli/cmdoptions.py Outdated Show resolved Hide resolved
@pypa pypa deleted a comment from pradyunsg Oct 23, 2018
@pypa pypa deleted a comment from pradyunsg Oct 23, 2018
@pypa pypa deleted a comment from pradyunsg Oct 23, 2018
@pypa pypa deleted a comment from pradyunsg Oct 23, 2018
@pypa pypa deleted a comment from pradyunsg Oct 23, 2018
@pypa pypa deleted a comment from pradyunsg Oct 23, 2018
@pypa pypa deleted a comment from pradyunsg Oct 23, 2018
@cjerdonek cjerdonek force-pushed the pip-no-cache-dir-var branch from cb24ca6 to 100d126 Compare October 25, 2018 09:03
@cjerdonek
Copy link
Member Author

I posted a new version that tries to use callbacks, as suggested by @pradyunsg. I'm not confident of the PR yet because of the interaction with the environment variables / config handling / etc, but all of the tests I wrote before still pass.

I also removed the issue numbers from the earlier commit message.

@cjerdonek cjerdonek force-pushed the pip-no-cache-dir-var branch 5 times, most recently from 88ec3c4 to d05355d Compare October 25, 2018 19:58
@cjerdonek
Copy link
Member Author

@pradyunsg This is ready to review again. Thanks for taking a look before!

pradyunsg
pradyunsg approved these changes Nov 8, 2018
View reviewed changes
src/pip/_internal/cli/cmdoptions.py Outdated
parser.values.cache_dir = False


# This value is used by OptionValues.cache_dir.
Copy link
Member

Choose a reason for hiding this comment

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

This comment can go away.

tests/unit/test_options.py
@@ -7,6 +8,23 @@
from tests.lib.options_helpers import AddFakeCommandMixin


@contextmanager
def assert_raises_message(exc_class, expected):
Copy link
Member

Choose a reason for hiding this comment

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

This is a nice helper; maybe in the future we should look into if this can be used elsewhere in the test suite.

@pradyunsg
Copy link
Member

Sorry for the delay -- I'd reviewed this earlier but never actually submitted the review.

@cjerdonek cjerdonek force-pushed the pip-no-cache-dir-var branch from d05355d to b94d719 Compare November 8, 2018 11:44
@cjerdonek
Copy link
Member Author

Thanks, @pradyunsg. I addressed your comment and squashed my commits (to delete my original unused approach from history).

@cjerdonek cjerdonek merged commit fc1b1e4 into pypa:master Nov 8, 2018
@cjerdonek cjerdonek deleted the pip-no-cache-dir-var branch November 8, 2018 22:19
@jakirkham jakirkham mentioned this pull request Nov 9, 2018
Closed
@burkestar burkestar mentioned this pull request Jan 22, 2019
Closed
@lock
Copy link

lock bot commented May 31, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label May 31, 2019
@lock lock bot locked as resolved and limited conversation to collaborators May 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pradyunsg pradyunsg pradyunsg approved these changes

@4383 4383 4383 approved these changes

Assignees
No one assigned
Labels
auto-locked Outdated issues that have been locked by automation C: cache Dealing with cache and files in it C: configuration Configuration management and loading
Projects
None yet
Milestone
19.0
Development

Successfully merging this pull request may close these issues.
3 participants
@cjerdonek @pradyunsg @4383