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

Update documentation and template for reviewers about tests #203

Merged
merged 4 commits into from
Apr 12, 2023

Conversation

cmarmo
Copy link
Member

@cmarmo cmarmo commented Mar 20, 2023

This pull request updates documentation and template for reviewers.
Closes #198.

Thanks for considering it.

@@ -102,6 +102,13 @@ in the review template, please also provide general comments addressing the foll

If you have your own relevant data/problem that you could use the package to solve, work through it with the package. You may find rough edges and use-cases the author didn't think about.

```{important}
Some maintainers do not ship the tests with their package.
This means that the dependencies for testing may be missing.
Copy link
Member

Choose a reason for hiding this comment

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

@cmarmo i'm curious if even if authors ship tests, would they add a tool like pytest as a dependency for the entire package?

Copy link
Member

Choose a reason for hiding this comment

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

so there would be a dep section in the pyproject.toml file for dependencies but those might not necessarily get installed with the package install ?

Copy link
Member Author

Choose a reason for hiding this comment

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

If I understand correctly you can define optional dependencies in pyproject.toml eg

[project.optional-dependencies]
test = ["pytest"]
doc = [
  "matplotlib",
  "click",
]

then install with eg

pip install mypackage[test]

Copy link
Member

Choose a reason for hiding this comment

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

thank you! i never knew that you could install that way (ie using [test] but learned about it today!

What do you think about us adding a note about how to install with tests using the example that you provide?

i've also made a note to clarify how to run tests in our package guide as well :)

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you think about us adding a note about how to install with tests using the example that you provide?

I think it could be helpful for packagers to know about the optional-dependencies metadata.

The keyword in the install command depends on the choice of the maintainer/packager.
In my example the command to install the doc dependencies is

pip install mypackage[doc]

but could have been docs for example.
If the maintainer adds optional-dependencies we might want to check that the install documentation is complete during the review.

@lwasser
Copy link
Member

lwasser commented Apr 12, 2023

ok i am going to merge this as is! it's important to have this discussion around tests in the guide. if we need to refine this a bit in the future or add more detail we can!

@lwasser
Copy link
Member

lwasser commented Apr 12, 2023

pre-commit.ci autofix

@lwasser lwasser merged commit 7e1cb1c into pyOpenSci:main Apr 12, 2023
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.

Suggest in review template to refer to test requirements
2 participants