Fix VIRTUALENV_PYTHON environment lookup
#1998
Conversation
The previous change which introduced the fallback discovery (pypa#1995) broke this behaviour.
Codecov Report
@@ Coverage Diff @@
## main #1998 +/- ##
==========================================
- Coverage 94.26% 94.25% -0.02%
==========================================
Files 86 86
Lines 4274 4280 +6
==========================================
+ Hits 4029 4034 +5
- Misses 245 246 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -31,6 +32,13 @@ def test_value_bad(monkeypatch, caplog, empty_conf): | |||
| assert "invalid literal" in caplog.messages[0] | |||
|
|
|||
|
|
|||
| def test_python_via_env_var(monkeypatch): | |||
| options = VirtualEnvOptions() | |||
| monkeypatch.setenv(str("VIRTUALENV_PYTHON"), str("python3")) | |||
There was a problem hiding this comment.
Can you add a test to validate that python2,python3 works as expected too?
There was a problem hiding this comment.
I extended the code to provide that functionality. Multi-value was split with newline so far, though it seems the only other multi-value field is extra-search-dir.
Let me know if you are happy with the approach taken or would prefer a different way to do this.
Unlike `VIRTUALENV_EXTRA_SEARCH_DIR` where the values are separated by newline, a comma is used for this value.
| @@ -39,6 +39,13 @@ def test_python_via_env_var(monkeypatch): | |||
| assert options.python == ["python3"] | |||
|
|
|||
|
|
|||
| def test_python_multi_value_via_env_var(monkeypatch): | |||
| options = VirtualEnvOptions() | |||
| monkeypatch.setenv(str("VIRTUALENV_PYTHON"), str("python3,python2")) | |||
There was a problem hiding this comment.
Wait, I did not realize that VIRTUALENV_EXTRA_SEARCH_DIR accepts newlines. Can we instead make this one use the same logic, and perhaps alter the logic for that to also accept the comma, not just newlines?
There was a problem hiding this comment.
Actually, what we have is correct, but instead of having different split character depending on the type, let's always accept both. I don't expect people to use comma in paths, so IMHO, this is alright.
There was a problem hiding this comment.
I am pushing a commit which implements this change. I opted for only accepting one split option at a time - so if the environment variable contains newlines, then we don't also accept commas. Do you agree with that decision?
I quickly went back to the legacy code as well, and it seems that newline support has only been added with the rewrite. The previous version had space separation for path names. Thus I would expect the backwards compatibility impact of this change to be minimal.
There was a problem hiding this comment.
That makes sense. Can you add a test demonstrating/validating this? Also, the documentation should explicitly state that only of the two will be accepted, and not both at the same time.
There was a problem hiding this comment.
Those tests have been added now.
| @@ -39,6 +39,13 @@ def test_python_via_env_var(monkeypatch): | |||
| assert options.python == ["python3"] | |||
|
|
|||
|
|
|||
| def test_python_multi_value_via_env_var(monkeypatch): | |||
| options = VirtualEnvOptions() | |||
| monkeypatch.setenv(str("VIRTUALENV_PYTHON"), str("python3,python2")) | |||
There was a problem hiding this comment.
That makes sense. Can you add a test demonstrating/validating this? Also, the documentation should explicitly state that only of the two will be accepted, and not both at the same time.
|
Thank you @gaborbernat! |
The previous change which introduced the fallback discovery (#1995) broke this behaviour.
I originally thought to change the
ListTypeclass which usesself.as_typeto cast the individual list values into the target value. At the moment the default for that type is alistwhich means the default behaviour for the class is to return a list of lists. But that became a bigger change than I'm comfortable doing on this code base, so I came up with this simpler change.Thanks for contributing, make sure you address all the checklists (for details on how see development documentation)!
tox -e fix_lint)docs/changelogfolder