Install package locally #76
Conversation
Prior to this change, the contributor instructions did not recommend installing the first-party `timezone_tools` package locally. This meant you had to pass a `PYTHONPATH` environment variable to pytest to run tests. This isn't a problem when running tests in `tox` as it automatically installs the first-party package as part of its set-up phase. This change updates the `makefile` to install the `timezone_tools` in editable mode, making it possible to run `pytest` (and other tools) from repo root.
codeinthehole
force-pushed
the
cc-backports
branch
from
34a492b to
ba7688e
Compare
| .PHONY:install_python_packages | ||
| install_python_packages: requirements.txt | ||
| # Install the `timezone_tools` package (in editable mode) and the development dependencies. | ||
| python3 -m pip install -r requirements.txt -e . |
There was a problem hiding this comment.
Am not wedded to having a makefile if you'd prefer to have this command in CONTRIBUTING.md instead. But I think it's probably a good idea to keep commonly-used commands in a makefile (or invoke tasks if we get there) to make contributors' lives easier. Especially once the command has a few options in it.
There was a problem hiding this comment.
We can change the command in the contributing guide -- I have no particular objection to that.
Although, I'm not sure I understand why everybody should need to install -e .. That does seem like something that's needed for a specific personal workflow. (again, not getting in your way there)
I would object to using GNU make as a task runner, though. I think it makes it less obvious what's going on, because contributors need to dig through the Makefile and it's funny syntax (what does "phony" mean? what is the indirection doing here? etc) to understand that the requirements are just in requirements.txt and if they have a preferred tool for managing their virtual environments, they just need to install that.
There was a problem hiding this comment.
I'm not sure I understand why everybody should need to install -e ..
To trace a parallel... When I am working on django-pg-migration-tools I can use nox to test. However, it takes too much time to run nox as it goes through the test matrix for all the permutations of Django and Python. It takes a lot of time even in parallel as I have more permutations than cores to run them.
The alternative is to run python -m pytest using the virtual environment on my local machine (no testing matrix, thus faster). It's nice to be able to do so when I am experimenting and want a quick feedback. Without installing the package in edit mode I'd have to do a pip install . every time I change a file I'm experimenting with before I run tests.
To trace a parallel...
That said, I am not sure what the current situation for this package is. It looks smaller from a testing matrix perspective and number of tests - CI takes 5s to run the testing matrix. If that is tolerable then devs could run that locally and it might not be that big of a deal.
There was a problem hiding this comment.
it takes too much time to run nox
That isn't the case for this project:
$ time tox -p
py310: OK ✔ in 1.29 seconds
py311: OK ✔ in 1.36 seconds
py313: OK ✔ in 1.37 seconds
py312: OK ✔ in 1.41 seconds
py310: OK (1.29=setup[0.90]+cmd[0.40] seconds)
py311: OK (1.36=setup[0.89]+cmd[0.47] seconds)
py312: OK (1.41=setup[0.88]+cmd[0.52] seconds)
py313: OK (1.37=setup[0.89]+cmd[0.48] seconds)
report: OK (0.18=setup[0.01]+cmd[0.08,0.09] seconds)
congratulations :) (1.63 seconds)
tox -p 1.88s user 0.70s system 145% cpu 1.778 totalEven without -p:
tox 1.63s user 0.52s system 81% cpu 2.626 total
Without installing the package in edit mode I'd have to do a pip install . every time I change a file
Oh, if we're installing the project then I understand why we'd want an editable install, rather than a static one, I just don't understand why it would be necessary to install it at all.
There was a problem hiding this comment.
Although, I'm not sure I understand why everybody should need to install -e .. That does seem like something that's needed for a specific personal workflow. (again, not getting in your way there)
I suppose this boils down to whether running pytest in the local (non-tox) virtualenv is a common or recommended practice that we should make possible with the standard installation process.
My instinct is that running pytest in this way is a more common practice than doing everything via tox.
While tox is fast in a small repo like this, it's still significantly slower. On my machine pytest takes 200-300ms to run the test suite while tox -e py312 takes 1-2 seconds. That's noticeable when doing TDD.
But crucially, does it do harm to have the package installed in editable mode?
It doesn't get in the way of people who only want to run tests via tox right? But not doing it is inconvenient for people who want to use the local pytest approach.
Plus it's inline with our how our CookieCutter repos are set-up, lowering the barrier to contributing to this repo for other devs.
That's what I think we should install the package locally.
marcelofern
left a comment
marcelofern
left a comment
There was a problem hiding this comment.
Very nice. I'd recommend a make help for self-documenting purposes too.
.PHONY:help
help:
@echo "Available targets:"
@echo " dev: Install all dev dependencies and this package in editable mode."I think that so far as the Makefile is only a shell-command runner (without logic), make is a pretty nice builder solution that most people will know, or can learn very quickly.
As I don't maintain this project directly I'll leave the approval for the people who do (-:
This PR:
makefilefor local-dev installation