Skip to content
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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Tox dotted env #39

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5a65ac4
feat: support for dotted env in tox
gpongelli Jan 30, 2023
fd6a29c
manage dotted env in update case
gpongelli Jan 30, 2023
9654bde
start removing the global
gpongelli Feb 4, 2023
7a6c81d
removed global var in favor of Version attribute
gpongelli Feb 4, 2023
877b77c
tests for version and tox
gpongelli Feb 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 24 additions & 4 deletions src/check_python_versions/sources/tox.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@


TOX_INI = 'tox.ini'
has_dotted_env = False


def get_tox_ini_python_versions(
Expand Down Expand Up @@ -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
Copy link
Owner

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.

Copy link
Contributor Author

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:

  1. if no env has dot, the update should put envs without dot
  2. if envs have at least one dot, the update should put envs with dot
  3. 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.

Copy link
Owner

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.

Copy link
Contributor Author

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.

Copy link
Owner

@mgedmin mgedmin Feb 6, 2023 •
edited
Loading

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.

Copy link
Owner

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?

Copy link
Contributor Author

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.

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 .

if '-' in env:
# e.g. py34-coverage, pypy-subunit
env = env.partition('-')[0]
if env.startswith('pypy'):
return Version.from_string('PyPy' + env[4:])
elif env.startswith('py') and len(env) >= 4 and env[2:].isdigit():
return Version.from_string(f'{env[2]}.{env[3:]}')
elif env.startswith('py'):
if len(env) >= 4 and env[2:].isdigit():
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:]}')
Copy link
Owner

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

else:
return None

Expand Down Expand Up @@ -157,6 +164,11 @@ def update_tox_envlist(envlist: str, new_versions: SortedVersionList) -> str:

trailing_comma = envlist.rstrip().endswith(',')

global has_dotted_env
has_dotted_env = False # reset global state before check
if '.' in envlist:
has_dotted_env = True

new_envs = [
toxenv_for_version(ver)
for ver in new_versions
Expand Down Expand Up @@ -223,7 +235,11 @@ def update_tox_envlist(envlist: str, new_versions: SortedVersionList) -> str:

def toxenv_for_version(ver: Version) -> str:
"""Compute a tox environment name for a Python version."""
return f"py{ver.major}{ver.minor if ver.minor >= 0 else ''}"
global has_dotted_env
_ret_str = f"py{ver.major}" \
f"{'.' if has_dotted_env else ''}" \
f"{ver.minor if ver.minor >= 0 else ''}"
return _ret_str
Copy link
Owner

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:



def should_keep(env: str, new_versions: VersionList) -> bool:
Expand All @@ -236,7 +252,11 @@ def should_keep(env: str, new_versions: VersionList) -> bool:
or 3.x version respectively in ``new_versions``.

"""
if not re.match(r'py(py)?\d*($|-)', env):
global has_dotted_env
_base_regex = r'py(py)?\d*($|-)'
if has_dotted_env:
_base_regex = r'py(py)?\d*(\.\d*)?($|-)'
if not re.match(_base_regex, env):
Copy link
Owner

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 True
if env == 'pypy':
return any(ver.major == 2 for ver in new_versions)
Expand Down
35 changes: 35 additions & 0 deletions tests/sources/test_tox.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,15 @@ def test_get_tox_ini_python_versions(tmp_path):
assert get_tox_ini_python_versions(tox_ini) == v(['2.7', '3.6', '3.10'])


def test_get_tox_ini_dotted_python_versions(tmp_path):
tox_ini = tmp_path / "tox.ini"
tox_ini.write_text(textwrap.dedent("""\
[tox]
envlist = py2.7,py3.6,py27-docs,pylint,py3.10
"""))
assert get_tox_ini_python_versions(tox_ini) == v(['2.7', '3.6', '3.10'])


def test_get_tox_ini_python_versions_syntax_error(tmp_path):
tox_ini = tmp_path / "tox.ini"
tox_ini.write_text(textwrap.dedent("""\
Expand Down Expand Up @@ -107,6 +116,32 @@ def test_update_tox_ini_python_versions():
""")


def test_update_tox_ini_dotted_python_versions():
fp = StringIO(textwrap.dedent("""\
[tox]
envlist = py2.6, py2.7
"""))
fp.name = 'tox.ini'
result = update_tox_ini_python_versions(fp, v(['3.6', '3.7', '3.10']))
assert "".join(result) == textwrap.dedent("""\
[tox]
envlist = py3.6, py3.7, py3.10
""")


def test_update_tox_ini_one_dotted_python_versions():
fp = StringIO(textwrap.dedent("""\
[tox]
envlist = py26, py2.7
"""))
fp.name = 'tox.ini'
result = update_tox_ini_python_versions(fp, v(['3.6', '3.7', '3.10']))
assert "".join(result) == textwrap.dedent("""\
[tox]
envlist = py3.6, py3.7, py3.10
""")


def test_update_tox_ini_python_syntax_error(capsys):
fp = StringIO(textwrap.dedent("""\
[tox
Expand Down