-
-
Notifications
You must be signed in to change notification settings - Fork 509
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
Fail on mismatched python spec attributes #2824
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Setting ``[testenv] basepython = python3`` will no longer override the Python | ||
interpreter version requested by a factor, such as ``py311``. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -95,6 +95,7 @@ def test_base_python_env_no_conflict(env: str, base_python: list[str], ignore_co | |
("py3", ["py2"], ["py2"]), | ||
("py38", ["py39"], ["py39"]), | ||
("py38", ["py38", "py39"], ["py39"]), | ||
("py38", ["python3"], ["python3"]), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This shouldn't conflict, but There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both should conflict. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're miss understanding. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That may be the case, but providing a specification of [tox]
minversion = 3.1
envlist = py{37,38,39,310,311}
[testenv]
basepython = python3 If I run
No warnings. Nothing. That's got to be incorrect behaviour, surely? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, so we have two options:
I'm inclined to go for option 1. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree. Option 1 is the way to go. Option 2 will result in different behaviour depending on the environment making tox less deterministic. That's A Bad Thing ™️ Without this change, the
So it seems I have implemented option 1, yes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens for env There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It passes, as expected.
|
||
("py310", ["py38", "py39"], ["py38", "py39"]), | ||
("py3.11.1", ["py3.11.2"], ["py3.11.2"]), | ||
("py3-64", ["py3-32"], ["py3-32"]), | ||
|
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.
Wrap at 120 and add
- by :user:`x`.
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.
Strong disagree. This is not a good way to express this. This config should hard fail for all envs expect py38.
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.
If those other envs care about a python version, they should specify so in their section; e.g. here, docs should set it to 3.8. Not setting it means I'm happy with any Python.
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.
Regarding the hard fail, currently
tox
does not hard fail with this configuration and it does the wrong thing (usingpython3
for all environments regardless of the version inferred from the factor). With this change, it does hard fail unless the user has explicitly setignore_base_python_conflict = true
. So I think what we're discussing here isn't actually this change itself, since this fixes an obvious bug.Regarding whether this is a good way to express this, requiring users duplicate
base_python
in every section makesbase_python
a special-case, no? For everything else, the idea is that[testenv]
allows you to set "default" values that you can later override (or extend) in[testenv:foo]
sections. tox itself does this for various things likecommand
:tox/tox.ini
Lines 27 to 35 in af4b558
tox/tox.ini
Lines 47 to 49 in af4b558
My question is why can we do this for things like
commands
,deps
, orpassenv
etc. but we should not be able to do it forbase_python
?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.
It's not a special case. Follows the same rule.
Would work. Where you're mistaken is that
py38
only setsbase_python
topy38
if at no point isbase_python
specified. If you set it attestenv
or[testenv:py38]
level that no longer activates.