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

Make use of -I in all Python invocations #11682

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

webknjaz
Copy link
Member

@webknjaz webknjaz commented Dec 7, 2023

This helps prevent testing the non-installed source. Additionally, the patch updates the docs to demonstrate a more predictable runpy-style invocation method.

@webknjaz webknjaz force-pushed the maintenance/isolated-runpy-invocations branch from 79c844b to d3322f5 Compare December 7, 2023 01:49
This helps prevent testing the non-installed source.
Additionally, the patch updates the docs to demonstrate a more
predictable runpy-style invocation method.
@webknjaz webknjaz force-pushed the maintenance/isolated-runpy-invocations branch from d3322f5 to 15e0e95 Compare December 7, 2023 02:04
Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @webknjaz.

I think the changes in CI are good (the pip/venv invocations).

For user documentation, I think we should keep without the -I, we can't guess what the user wants, for example it will break many of my works projects to use -I. We can perhaps add a note somewhere about it.

For our test suites, it will break a few things we do in tox.ini, for example setting PYTHONWARNDEFAULTENCODING=1

@webknjaz
Copy link
Member Author

webknjaz commented Dec 8, 2023

For user documentation, I think we should keep without the -I, we can't guess what the user wants, for example it will break many of my works projects to use -I. We can perhaps add a note somewhere about it.

@bluetech the docs was my primary motivation originally: I know that a lot of people make this mistake because it's obvious, sometimes. So in the patch, I didn't fully remove the mentions of -m but flipped the default example to behave closer to the way just pytest does — it seems to be more intuitive this way. It does make sense to show just -m in the section that explicitly suggests importing from a neighboring directory, though.

For our test suites, it will break a few things we do in tox.ini, for example setting PYTHONWARNDEFAULTENCODING=1

Would changing those to -X be acceptable?

@bluetech
Copy link
Member

I think we shouldn't use -I in the user docs because it isn't really meant for it:

-E - users often set envvars like PYTHONPATH, PYTHONWARNINGS etc, they shouldn't be ignored (and plain pytest doesn't).

-P users sometimes intentionally rely on it, we do intentionally document python -m pytest as a way to achieve that. But I think it's worth adding a note in the docs about python -Pm pytest, in case need to use -m but don't want to sys.path behavior.

-s users may rely on it, it's a reasonable thing to install pytest to user site.

Would changing those to -X be acceptable?

I think the right fix here is to just remove the install_command line (and also the download line). It was added in 8215625 but is no longer necessary. According to tox docs, the default command is python -I -m pip install {opts} {packages} so already isolated 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants