-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add option to list config files with pip config #8096
Conversation
76162b6
to
e098e60
Compare
To move the PR ahead, I have implemented the first suggestion provided in #6741 (comment). (Naming the command as The output now looks like:
Hi @xavfernandez , The only thing missing in the output in your example which I implemented in the latest commit is |
d441f4e
to
99be5e3
Compare
Hi @xavfernandez , @pradyunsg Any suggestions on the next steps to do here, in order to move this PR ahead and get this approved/merged, especially w.r.t #8096 (comment) are welcome. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this, although the naming of the command and functions should be updated, to more directly reflect what they're for. :)
30bc04a
to
39432a6
Compare
f8a44e9
to
9ed61dc
Compare
I have also gone ahead and added tests for this feature for everything except to test for system-wide configuration file which falls under I think we can neither write to system-wide files which might have permissions defined in a way that the tox virtualenv cannot write to those locations, and we cannot patch those files as well since Any suggestions on if there are other ways to achieve this is welcome. The current output for the command can be found at the PR description. |
7c71c59
to
47c85f7
Compare
Hi @pradyunsg This should be good to go except a test for verifying set global options inside a virtualenv, which I am unsure how to do (#8096 (comment)). Also the Travis CI has passed, but the status is stuck at Pending 😞 |
9ad9811
to
84d6719
Compare
I added a test in 84d6719 which verifies that the global configuration file is correctly identified for different platforms. With that, I think that this PR is implementation complete, and should be ready for further reviews. |
84d6719
to
286c5f4
Compare
Okay so the CI is green now too after rebasing to current master as well. The PR is ready for further reviews and approval/merge. |
I think this PR is feature complete, and I would really appreciate if you could take a look at this, and provide further reviews. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love it 👍
Ideally, we should also help identify invalid/unknown options or environment variable, something like:
$ PIP_TOTO=titi pip config debug
env_var:
PIP_TOTO='titi'
env:
global:
/etc/xdg/pip/pip.conf, exists: False
/etc/pip.conf, exists: False
site:
/home/xafer/.virtualenvs/pip/pip.conf, exists: False
user:
/home/xafer/.pip/pip.conf, exists: True
global.disable-pip-version-check: 1
global.timout: 10
PIP_TOTO is not a known configuration environment variable
global.timout is not a known configuration key (did you mean global.timeout ?)
but this can definitely happen in a followup PR :)
Thanks @xavfernandez . This should be ready to merge.
That's a very good idea, and I think this can definitely be handled in a separate issue/PR. |
cc777ab
to
056f119
Compare
Hi @pradyunsg We should be good to merge this unless there are more reviews needed here, post this I will work on #8096 (comment) |
Thanks a lot @deveshks 👍 |
Fixes and closes #6741
Add option to list config files per variant and whether they actually exist
This is a preliminary output, but we need to decide on some things before we finalize something
Edit: An updated format with all values for a config file listed under it is now added:
If
PIP_CONFIG_FILE
is setIf
PIP_CONFIG_FILE
is not set: