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

Readme claims test dependencies are installed automatically #1652

Closed
EliahKagan opened this issue Sep 10, 2023 · 0 comments · Fixed by #1654
Closed

Readme claims test dependencies are installed automatically #1652

EliahKagan opened this issue Sep 10, 2023 · 0 comments · Fixed by #1654

Comments

@EliahKagan
Copy link
Contributor

EliahKagan commented Sep 10, 2023

README.md says:

The list of dependencies are listed in ./requirements.txt and ./test-requirements.txt. The installer takes care of installing them for you.

That is not accurate, because while dependencies listed in test-requirements.txt are included in the list passed as tests_require in setup.py, this does not actually cause them to be installed. As README.md later notes, they currently have to be installed with a command like pip install -r test-requirements.txt. That makes the above text in the readme misleading, because this is not the case for requirements.txt dependencies, which do not have to be installed manually.

(Two related things: The CI workflows currently must install from requirements.txt explicitly because they do not install the project itself. The test_installation unit test installs from requirements.txt before installing the project itself, which should probably not be done, because this makes it so that test would not fail if installing GitPython failed to entail installation of its dependencies, which could arise as a regression in a change to setup.py.)

There are at least three distinct ways this documentation bug could be fixed:

  • Remove the claim that the installer takes care of this, while making no other changes.
  • Remove the claim that the installer takes care of this, and remove the test_require keyword argument from the setup call in setup.py.
  • Use an extra instead of tests_require so that there really is a way to install the package such that not only are the test dependencies downloaded but they are also installed automatically in the environment in which GitPython and its regular dependencies are installed. This has the substantial disadvantage that it is somewhat unintuitive to have an extra providing dependencies for functionality that is not itself part of the package people download from PyPI. But it would have the the advantage that it would fully achieve what appears to be intended or at least aspired to and that, depending on what other changes are being made, it might make the dependency installation procedure faster and simpler.

I've opened #1654, which fixes this as well as some other issues. I took the approach of making a test extra, but as noted in the PR description, that can be changed, including to either of the other two approaches I've suggested here.

@EliahKagan EliahKagan changed the title Readme claims tests dependencies are installed automatically Readme claims test dependencies are installed automatically Sep 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants