-
-
Notifications
You must be signed in to change notification settings - Fork 286
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
DOC: Proposed updates to contributor guide. #2513
base: main
Are you sure you want to change the base?
DOC: Proposed updates to contributor guide. #2513
Conversation
@@ -92,12 +92,11 @@ the following:: | |||
$ mkdir -p ~/pyenv/zarr-dev | |||
$ python -m venv ~/pyenv/zarr-dev | |||
$ source ~/pyenv/zarr-dev/bin/activate | |||
$ pip install -r requirements_dev_minimal.txt -r requirements_dev_numpy.txt | |||
$ pip install -e .[docs] | |||
$ pip install -e .[test,docs] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There don't appear to be requirements files anymore, so I assume that the optional-dependencies
is the intended replacement - not sure how these intersect requirements_dev_minimal
and requirements_dev_numpy
but these appear to be the minimal ones to run the tests and build the docs!
|
||
To verify that your development environment is working, you can run the unit tests:: | ||
|
||
$ python -m pytest -v zarr | ||
$ python -m pytest -v tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there's been a src
/tests
split since this was written, so the original recommendation no longer applies. This should work and be portable, but feel free to replace with whichever incantation is preferred!
@@ -149,7 +148,7 @@ and invoke:: | |||
Some tests require optional dependencies to be installed, otherwise | |||
the tests will be skipped. To install all optional dependencies, run:: | |||
|
|||
$ pip install -r requirements_dev_optional.txt | |||
$ pip install pytest-doctestplus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see pytest-doctestplus
listed anywhere in the optional dependencies, and I don't know what else may have been in requirements_dev_optional
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be pro putting this in the test dependencies section, and then not having any optional dependencies for testing? That way it's a bit simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - this looks great. Just one suggestion to improve things by dropping having optional test dependencies.
I just took a walk through the contributor guide and noticed it appears to be out-of-sync with the current structure of the repo.
I went ahead and made some minor tweaks to get things working again, but someone with knowledge of any recent restructuring should give the whole document a once-over!