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

Add docs on how to run tests #53

Merged
merged 6 commits into from
Mar 24, 2024
Merged

Add docs on how to run tests #53

merged 6 commits into from
Mar 24, 2024

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Mar 23, 2024

I couldn't find docs on how to run tests locally, so I had to take a few guesses. I'm suggesting a section at the end of the README.

Copy link
Collaborator

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

I think this can all be replaced with pip install --editable . which is a pytest best practice.

Or more compactly as demonstrated in the GitHub Action pip install --editable ".[dev]"

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@@ -1,6 +1,7 @@
import glob
import os
import subprocess
import sys
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not required if you have a venv.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it hurt though? If not, it seems to me better to keep it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. It hurts. Please read Why do virtual environments exist? from a long-term core Python developer and head of Python development for VSCode... https://snarky.ca/how-virtual-environments-work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the end, whether you type python after activation or .venv/bin/python makes no difference to Python

Hum I don't know if I'm reading that right, but it seems to me this implies it would not hurt, as it says it makes no difference, no? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

It says that the goal of using a virtualenv to avoid installing your dependencies into your system Python.
Doing so can cause dependency conflicts that can become unresolvable when multiple projects are installed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I do see the upsides of using a virtualenv, much less troublesome than spawning a docker container. But I thought you were saying “it hurts to use sys.executable instead of "python"”, and – I guess this where I’m missing something – I don’t see how that relates to virtualenv or how it would hurt.

@@ -1,6 +1,7 @@
import glob
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not required if you have a venv.

@cclauss cclauss changed the title add docs on how to run tests, use sys.executable instead of "python" Add docs on how to run tests Mar 24, 2024
@cclauss cclauss merged commit 27fbbaf into main Mar 24, 2024
1 check failed
@cclauss cclauss deleted the onboarding-docs branch March 24, 2024 12:15
@cclauss
Copy link
Collaborator

cclauss commented Mar 24, 2024

The reason for the shift to sys.executable is to enable skipping the best practice of making a venv (which links python to python3 in Linux and macOS and to py in Windows). Let's reenforce the use of the best practice in our instructions.

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.

2 participants