-
-
Notifications
You must be signed in to change notification settings - Fork 30.6k
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
[doc] Document VIRTUAL_ENV environment variable (GH-21970) #21970
[doc] Document VIRTUAL_ENV environment variable (GH-21970) #21970
Conversation
Thanks @andresdelfino for the PR, and @vsajip for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9. |
Thanks @andresdelfino for the PR, and @vsajip for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8. |
GH-24362 is a backport of this pull request to the 3.9 branch. |
GH-24363 is a backport of this pull request to the 3.8 branch. |
(cherry picked from commit 3584d4b) Co-authored-by: Andre Delfino <adelfino@gmail.com>
(cherry picked from commit 3584d4b) Co-authored-by: Andre Delfino <adelfino@gmail.com>
(cherry picked from commit 3584d4b) Co-authored-by: Andre Delfino <adelfino@gmail.com>
@vsajip - sorry to drag this up, but I believe this change is incorrect. It would be dangerous to use an environment variable to determine if a virtual environment is being used or not. I would never do this in production, and instead would check This recommendation is indeed already explicitly stated in the same file:
A simple counter example to the documentation would be:
I therefore recommend that this commit be reverted. I would be happy to file a bug if that is required to revert. |
Rather than doing that, I would propose to change the paragraph as follows:
|
I take your lead on this, but to me that sounds like quite a lot of words for not a lot of new detail. I definitely think the page in question could be reshuffled a bit to make this concise and easier to find the information quickly. Would you like me to submit a proposal? |
By all means. |
I think we shouldn't remove the mention to VIRTUAL_ENV (reverting this commit would have that effect), as it is helpful. Of course, I'm all for improving the correctness of the docs. |
Well, I made my suggestion in the way that I did because it
and it may be wordier than it needs to be, but I was thinking of spelling things out for any inexperienced users / users new to virtual environments. |
Automerge-Triggered-By: @vsajip