-
-
Notifications
You must be signed in to change notification settings - Fork 12
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
Tox dotted env #39
base: master
Are you sure you want to change the base?
Tox dotted env #39
Conversation
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.
Thank you!
@@ -99,13 +100,19 @@ def tox_env_to_py_version(env: str) -> Optional[Version]: | |||
If the environment name has dashes, only the first part is considered, | |||
e.g. py34-django20 becomes '3.4', and jython-docs becomes 'jython'. | |||
""" | |||
global has_dotted_env | |||
has_dotted_env = False # reset global state before check |
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.
Can we avoid global state please?
IIIRC I had a function to parse tox envlist names and convert them to Version objects. It should handle optional dots.
The update functions should instead check whether the envlists were using dots or not, just like they currently check whether the envlist uses spaces after commas.
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.
the main way I followed when developing was to choose the format to be used in the whole file:
- if no env has dot, the update should put envs without dot
- if envs have at least one dot, the update should put envs with dot
- in mixed case env, it's forced to "change all env with dot"
I know that point 3 is little risky, because developer should update the environments in the remaining part of the file.
I've no idea how to keep "mixed style env" and I don't know what's your point.
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.
Sure, I agree with points (1) and (2) and don't care about point (3) -- normalizing to one style is fine, I don't particularly care which style it should be.
My point was that this doesn't need a global switch, a local check inside the update_...() function that then passes the detected style to any helpers should suffice.
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.
perhaps I didn't express correctly the drawback about third point : in case of this starting state
[tox]
envlist = py3.8, py3.9, py310, py311
...
[testenv]
setenv =
{py3.8, py3.9, py310, py311}: <<do something here>>
commands =
py3.8: <<do something here>>
py3.9: <<do something here>>
py310: <<do something here>>
py311: <<do something here>>
if the dot isn't stored in some object that identifies what's found in envlist (Version object are created), it can happen that after the update the state becomes (e.g. deciding to force dot removal way)
[tox]
envlist = py38, py39, py310, py311
...
[testenv]
setenv =
{py3.8, py3.9, py310, py311}: <<do something here>> <<< HERE NO CHANGES, misalignment with testenv
commands =
py3.8: <<do something here>> <<< HERE NO CHANGES, misalignment with testenv
py3.9: <<do something here>> <<< HERE NO CHANGES, misalignment with testenv
py310: <<do something here>>
py311: <<do something here>>
as you can see, the envlist is updated removing the dots on 3.8 and 3.9, but the remaining part of the file was not touched , and this led to issue if no review is done AFTER check-python-version was run.
Instead, keeping the 'dot' information as I did, the already existing version into envlist are not changed, so everything still match in testenv section.
I hope that his had clarified the reason about my recent changes.
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.
I see. I would hope that tox is smart enough to consider e.g. py38
and py3.8
factors as equivalent, so the configuration would continue to work despite the misalignment.
My view on this remains: preserving a preexisting mixture of styles is extra effort, I don't mind if it's done, but it's not a requirement for the PR to be merged.
Adding mutable global state or a thematically-unrelated attribute to a generic Version class are deal-breakers.
If I wanted to preserve a mixture of styles (which I don't want to do), I'd create a supplementary mapping of Version -> str in update_tox_envlist:
old_spelling = {}
for env in parse_envlist(envlist):
ver = tox_env_to_py_version(env)
if ver:
old_spelling[ver] = env
and then reuse it when computing new_envs:
new_envs = [
old_spelling.get(ver, toxenv_for_version(ver))
for ver in new_versions
]
This way the state is local and Version doesn't need to be extended with complications that affect equality comparisons.
UPDATE: this doesn't consider keeping a consistent style for newly-added versions, for which I'd do
old_spelling = {}
use_dots = False
for env in parse_envlist(envlist):
ver = tox_env_to_py_version(env)
if ver:
old_spelling[ver] = env
if f'{ver.major}.{ver.minor}' in env: # py310-django2.7 does not use dots, py3.10 does
use_dots = True
new_envs = [
old_spelling.get(ver, toxenv_for_version(ver, use_dots=use_dots))
for ver in new_versions
]
and now I'm thinking about py310-numpy3.10
and how that would mistakenly trigger the use_dots=True heuristic and maybe the check should be env.startswith(f'py{ver.major}.{ver.minor}')
?
What about pypy? Does tox support things like pypy3.11
? Do we need to make tox_env_to_py_version() return a tuple (ver, uses_dots)?
I keep almost reconsidering Version.use_dots, but then I think about comparisons again and, no.
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.
I'm more worried about the # Try to preserve braced groups
branch. Does that continue to work? Does it support dotted versions? E.g. will
[tox]
envlist = py{3.9,3.10}{,-foo}
be converted to
[tox]
envlist = py{3.9,3.10,3.11}{,-foo}
after a check-python-versions --add 3.11
?
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.
I see. I would hope that tox is smart enough to consider e.g.
py38
andpy3.8
factors as equivalent, so the configuration would continue to work despite the misalignment.
I've just done some tests, seems not so smart as you hope:
[tox:tox]
envlist = py3.10
[testenv]
commands =
py3.10: echo dotted
py310: echo not-dotted
echo after
if I run poetry run tox
, the message I see on screen are dotted
and after
. Switching to envlist = py310
I see on screen are not-dotted
and after
.
_ret_str = f"py{ver.major}" \ | ||
f"{'.' if has_dotted_env else ''}" \ | ||
f"{ver.minor if ver.minor >= 0 else ''}" | ||
return _ret_str |
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.
Instead of global state this should take an argument
def toxenv_for_version(ver: Version, use_dots: bool = False) -> str:
_base_regex = r'py(py)?\d*($|-)' | ||
if has_dotted_env: | ||
_base_regex = r'py(py)?\d*(\.\d*)?($|-)' | ||
if not re.match(_base_regex, env): |
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.
The regexp should always allow an optional dot. There's no reason to do that only if you know that some of the env names use it.
I think r'py(py)?(\d[.])?\d*($|-)'
would work.
(I usually use [.]
instead of \.
to match literal dots, but that's a personal quirk. There's no real reason to prefer it in this context.)
return Version.from_string(f'{env[2]}.{env[3:]}') | ||
if '.' in env: | ||
has_dotted_env = True | ||
return Version.from_string(f'{env[2]}.{env[4:]}') |
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.
We may have reached the point where manual substring checks are no longer simpler than a regular expression.
Besides, I think this mishandles corner cases like py.29
(should return None) or py10.20
(should return Version('10.20')
).
removed global variable in favor of an attribute into Version. Actual solution keeps the dot in envs that have it, and do not add dot to them that haven't it. |
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.
I'm sorry if I gave the mistaken impression that I want to preserve mixed-style envlists. I don't. I just didn't want a global variable.
What I want is the update_tox_ini_python_versions()
, or, actually, update_tox_envlist()
to check whether the existing envlist uses dots or not, and then format new_envs
accordingly.
@@ -43,9 +43,10 @@ class Version(NamedTuple): | |||
major: int = -1 # I'd've preferred to use None, but it complicates sorting | |||
minor: int = -1 | |||
suffix: str = '' | |||
has_dot: bool = False |
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.
This doesn't belong here. It has nothing to do with what a Version represents.
if not has_dot: | ||
has_dot = [False] * len(versions) | ||
return [Version.from_string(v, has_dot=d) | ||
for v, d in zip(versions, has_dot)] |
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.
Why is this needed? Is Version(3, 11, has_dot=True) != Version(3, 11, has_dot=False)
? No, that's going to be causing confusion and bugs.
I wait answer about this comment before doing anything. |
#39 (comment) has my latest thoughts (and there's an update where I edited the comment -- IIRC GitHub doesn't sent notification emails for comment edits.) |
manage dotted env for tox
envlist
closes #38