Tox dotted env #39

gpongelli · 2023-01-30T10:07:07Z

manage dotted env for tox envlist

closes #38

mgedmin

Thank you!

mgedmin · 2023-02-02T13:38:07Z

src/check_python_versions/sources/tox.py

@@ -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


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.

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.

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.

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.

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.

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?

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.

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 .

mgedmin · 2023-02-02T13:42:44Z

src/check_python_versions/sources/tox.py

+    _ret_str = f"py{ver.major}" \
+               f"{'.' if has_dotted_env else ''}" \
+               f"{ver.minor if ver.minor >= 0 else ''}"
+    return _ret_str


Instead of global state this should take an argument

def toxenv_for_version(ver: Version, use_dots: bool = False) -> str:

mgedmin · 2023-02-02T13:45:13Z

src/check_python_versions/sources/tox.py

+    _base_regex = r'py(py)?\d*($|-)'
+    if has_dotted_env:
+        _base_regex = r'py(py)?\d*(\.\d*)?($|-)'
+    if not re.match(_base_regex, env):


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.)

mgedmin · 2023-02-02T13:51:12Z

src/check_python_versions/sources/tox.py

+            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:]}')


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')).

gpongelli · 2023-02-04T20:33:20Z

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.

mgedmin

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.

mgedmin · 2023-02-06T07:42:32Z

src/check_python_versions/versions.py

@@ -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


This doesn't belong here. It has nothing to do with what a Version represents.

mgedmin · 2023-02-06T07:45:20Z

tests/sources/test_tox.py

+    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)]


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.

gpongelli · 2023-02-06T09:24:55Z

I wait answer about this comment before doing anything.

mgedmin · 2023-02-06T14:36:15Z

#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.)

gpongelli added 2 commits January 30, 2023 10:08

feat: support for dotted env in tox

5a65ac4

manage dotted env in update case

fd6a29c

gpongelli mentioned this pull request Jan 30, 2023

support for tox into setup.cfg #35

Open

mgedmin requested changes Feb 2, 2023

View reviewed changes

gpongelli added 3 commits February 4, 2023 19:49

start removing the global

9654bde

removed global var in favor of Version attribute

7a6c81d

tests for version and tox

877b77c

mgedmin requested changes Feb 6, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tox dotted env #39

Tox dotted env #39

gpongelli commented Jan 30, 2023 •

edited

Loading

mgedmin left a comment

mgedmin Feb 2, 2023

gpongelli Feb 4, 2023

mgedmin Feb 6, 2023

gpongelli Feb 6, 2023

mgedmin Feb 6, 2023 •

edited

Loading

mgedmin Feb 6, 2023

gpongelli Feb 7, 2023

mgedmin Feb 2, 2023

mgedmin Feb 2, 2023

mgedmin Feb 2, 2023

gpongelli commented Feb 4, 2023

mgedmin left a comment •

edited

Loading

mgedmin Feb 6, 2023

mgedmin Feb 6, 2023

gpongelli commented Feb 6, 2023

mgedmin commented Feb 6, 2023

Tox dotted env #39

Are you sure you want to change the base?

Tox dotted env #39

Conversation

gpongelli commented Jan 30, 2023 • edited Loading

mgedmin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgedmin Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpongelli commented Feb 4, 2023

mgedmin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpongelli commented Feb 6, 2023

mgedmin commented Feb 6, 2023

gpongelli commented Jan 30, 2023 •

edited

Loading

mgedmin Feb 6, 2023 •

edited

Loading

mgedmin left a comment •

edited

Loading