Don't warn when PYTHONPATH is present but empty. #1092
Conversation
Some users (e.g. me :) have an always set [but empty] PYTHONPATH (for reasons like supporting zsh's typeset -T). Since Python's own behavior is the same in this case as having the env var unset, tox shouldn't warn.
| @@ -387,7 +387,7 @@ def ensure_pip_os_environ_ok(self): | |||
| os.environ.pop(key, None) | |||
| if "PYTHONPATH" not in self.envconfig.passenv: | |||
| # If PYTHONPATH not explicitly asked for, remove it. | |||
| if "PYTHONPATH" in os.environ: | |||
| if os.environ.get("PYTHONPATH"): | |||
There was a problem hiding this comment.
note that this is a behavior change allowing the empty variable to pass trough
There was a problem hiding this comment.
Yeah I considered that (and popping it out) but couldn't think of a decent reason why it'd matter.
Happy to change if someone does though.
There was a problem hiding this comment.
Well, at least it will make the env larger, and therefore I think it should not be added in the first place - it is nicer to have less in env etc.
There was a problem hiding this comment.
let's err on the side of caution and remove it anyway. can dedent line 395 and change it to os.environ.pop('PYTHONPATH', None)
|
zsh typeset support? feels to me like you're trying to fix a zsh bug in tox. |
|
It's not a zsh bug, I was giving some background on how I found this issue
(because I happen to have the variable always set).
Judge the patch on its own merit.
…On Mon, Nov 26, 2018, 09:15 Bernát Gábor ***@***.*** wrote:
zsh typeset support? feels to me like you're trying to fix a zsh bug in
tox.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1092 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAUIXhSd8kNgOu_zfXylbNAC5wC-Nv_gks5uy_eQgaJpZM4YzBag>
.
|
|
Without context, I'm tempted to reject this. Setting the env var as empty is likely to confuse users so warning on it seems a best practice (even if python silently swallows this kind of odd usage). |
|
Sorry, what? Which users is this confusing? A user has set PYTHONPATH to be empty. This is valid, has documented behavior equivalent to not having it set at all. I gave a reason why someone might be doing this, but equally well a user could be writing All this does is silence a warning for the case that it isn't needed. The purpose of the warning is to let the user know that their environment variable is being ignored (which by the way tox isn't even consistent about, but fine). In this case, it's not ignored, they're getting the same behavior, so don't pollute their stdout with red messages. Probably should have written all that elaboration earlier I guess, but if you still disagree it's really not worth pursuing such a small fix so feel free to close. |
|
Can you please link to |
|
It appears the behavior for an empty PYTHONPATH changed in Python 3.4 for the
|
|
Sorry, lucky me caught the flu and has been completely out of commission for the last week. Will have to catch up with whatever comments here. |
|
@Julian do you plan to finish this? |
|
Seemingly unlikely at the minute unfortunately, too many other things. Will close and send a new PR if I do get a moment to get back to it. |
|
Let's keep it open then, I might finish it. |
|
So the forth going goal for this PR is to only warn on versions >= 3.4? Just looking for a summary of what we've established as the best function for this PR |
|
Yes 👍 |
|
Excellent! Thank you, @gaborbernat 😊 |
|
superseded with #1189 |
Some users (e.g. me :) have an always set [but empty] PYTHONPATH
(for reasons like supporting zsh's typeset -T). Since Python's own
behavior is the same in this case as having the env var unset,
tox shouldn't warn.
Thanks for contributing a pull request!
If you are contributing for the first time or provide a trivial fix don't worry too
much about the checklist - we will help you get started.
Contribution checklist:
(also see CONTRIBUTING.rst for details)
in message body
<issue number>.<type>.rstfor example (588.bugfix.rst)<type>is must be one ofbugfix,feature,deprecation,breaking,doc,miscsuperuser."CONTRIBUTORS(preserving alphabetical order)