Skip to content

fix(parser): Fix --discover parsed incorrectly from env#3274

Merged
gaborbernat merged 4 commits intotox-dev:mainfrom
mimre25:fix-tox_discover-env-parsing
Apr 26, 2024
Merged

fix(parser): Fix --discover parsed incorrectly from env#3274
gaborbernat merged 4 commits intotox-dev:mainfrom
mimre25:fix-tox_discover-env-parsing

Conversation

@mimre25
Copy link
Copy Markdown
Contributor

@mimre25 mimre25 commented Apr 26, 2024

This PR fixes the parsing of the --discover flag and improves the docs of it to avoid confusion between paths and executables.

The flag was missing the type specification (of_type) and thus parse.get_env_var did not execute the branch for list, but rather for single variables.
The fallback in Converter led to a call to list("abc"), which resulted int a list of characters (["a", "b", "c"]).

  • ran the linter to address style issues (tox -e fix)
  • wrote descriptive pull request text
  • ensured there are test(s) validating the fix
  • added news fragment in docs/changelog folder
  • updated/extended the documentation

Closes #3272

@mimre25
Copy link
Copy Markdown
Contributor Author

mimre25 commented Apr 26, 2024

@gaborbernat @jugmac00 Right now the changes fix the parsing of the TOX_DISCOVER env variable.

It's important to follow the format described in set-cli-flags-via-environment-variables and separate the list with ; instead of .

However, one thing that I countered is either a confusingly worded documentation or another bug with discover:

--discover PATH - for Python discovery first try the Python executables under these paths (default: [])

emphasis by me

For me this reads as if tox would expect a path (/foo/bar/) under which one or more python executables exist (/foo/bar/python).
However, this is not the case.
Instead tox expects --discover to get one or more python executables (/foo/bar/python).

To illustrate this (using the same project as in the issue):

✗ tox -q -e py310 --discover /home/martin/micromamba/envs/py310/bin/  
  py310: SKIP (0.01 seconds)
  evaluation failed :( (0.08 seconds)                                                                                            [0,28s]
✗ tox -q -e py310 --discover /home/martin/micromamba/envs/py310/bin/python
========================================================= test session starts ==========================================================
platform linux -- Python 3.10.12, pytest-8.1.1, pluggy-1.5.0
cachedir: .tox/py310/.pytest_cache
rootdir: /home/martin/workspace/ansible-lint-empty-lines-between-tasks
configfile: pyproject.toml
testpaths: test
collected 4 items                                                                                                                      

test/test_rule.py ....                                                                                                           [100%]

========================================================== 4 passed in 1.55s ===========================================================
  py310: OK (24.92 seconds)
  congratulations :) (24.99 seconds)     

I propose to rewrite the docs here to be more specific that --discover expects a list of python executables for two reasons:

  1. Smaller change 🤓
  2. It's backwards compatible, ie, wherever --discover (or $TOX_DISCOVER) is working now, it will continue to work.

What are your thoughts? Should I update the docs?

@gaborbernat
Copy link
Copy Markdown
Member

Yes please.

mimre25 added 3 commits April 26, 2024 19:23
The flag was missing the type specification (`of_type`) and thus
parse.get_env_var did not execute the branch for list, but rather for
single variables.
The fallback in Converter led to a call to `list("abc")`, which
resulted int a list of characters (["a", "b", "c"]).
The flag expects a list of python executables, but not paths.
Eg `--discover /foo/bar/python` is good, `--discover /foo/bar` or
`--discover /foo/bar/` won't work.
@mimre25 mimre25 force-pushed the fix-tox_discover-env-parsing branch from b3b1a07 to 7d73641 Compare April 26, 2024 17:31
@mimre25 mimre25 marked this pull request as ready for review April 26, 2024 17:32
@mimre25 mimre25 requested a review from gaborbernat as a code owner April 26, 2024 17:32
@mimre25
Copy link
Copy Markdown
Contributor Author

mimre25 commented Apr 26, 2024

@gaborbernat this is ready 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TOX_DISCOVER not working (micromamba)

2 participants