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.

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

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

Merged
merged 4 commits into from Apr 26, 2024

Conversation

mimre25
Copy link
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
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
Member

Yes please.

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
Copy link
Contributor Author

mimre25 commented Apr 26, 2024

@gaborbernat this is ready 🙂

@gaborbernat gaborbernat merged commit c54dfbd into tox-dev:main Apr 26, 2024
25 checks passed
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