Skip factors that don't start with an alphabetic character #2764
Conversation
A numeric factor in an environment name can wrongly cause a "conflicting factors" error. Fix tox-dev#2657 .
| @@ -128,6 +128,9 @@ def default_base_python(self, conf: Config, env_name: str | None) -> list[str]: | |||
| def extract_base_python(env_name: str) -> str | None: | |||
| candidates: list[str] = [] | |||
| for factor in env_name.split("-"): | |||
| if not factor[0].isalpha(): | |||
There was a problem hiding this comment.
Wouldn't this break names like 3.7 that is valid (and used https://github.com/psycopg/psycopg/actions/runs/3642624184/jobs/6149941052) ? Or 3?
There was a problem hiding this comment.
@gaborbernat Sorry for the late reply, holiday season!
I've looked at your example (had to go back to commit psycopg/psycopg@14bfb96 because they just removed their use of tox) and if their use of 3.7 as environment name was supposed to run tests under Python 3.7 it didn't work under tox 3 (see https://gist.github.com/nsoranzo/a291d87bbfa43c37a9c01a6aaf0d887b , where tox3 creates the 3.7 environment using Python 3.8) and I don't think it was never documented as supported, only py37.
There was a problem hiding this comment.
It is supported with tox 4 though 🤔so we should add this feature by supporting it. 😅
There was a problem hiding this comment.
tox 4 has been use in for just 20 days, this change is not documented (what environment factor may now be used as Python specifier? py37, 3.7, 37, 3?) and, most importantly, breaks environment names (as reported in #2657) which have been valid for years.
I don't think we can implement this feature without breaking some tox configuration that was working under the assumption that a number as environment factor is not interpreted as a Python specifier, but happy to help if you have a reasonable specification in mind.
There was a problem hiding this comment.
py37, 3.7, 37, 3
mostly these 🤔
There was a problem hiding this comment.
An easy fix will be that if we find 3.x.y, 3.x, 3x, or 3 factors, we still accept those here and pass them through but not other numbers.
|
Replaced by #2849 (review) |
|
@nsoranzo In summary, #2849 reverts to a modified version of the behaviour found in tox 3. The only changes are that more interpreter implementations are supported ( |
|
@gaborbernat Sorry I didn't have time to follow up on your suggestions! @stephenfin Thanks a lot for working on this and for your explanation, my issue is indeed fixed! 🎉 |
A numeric factor in an environment name can wrongly cause a "conflicting factors" error.
Fix #2657 .
Thanks for contribution
Please, make sure you address all the checklists (for details on how see
development documentation)!
tox -e fix)docs/changelogfolder