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

Split test dependencies into dev and test dependencies. #12341

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

picnixz
Copy link
Member

@picnixz picnixz commented May 1, 2024

When (and if) PEP 735 is accepted, we can probably simplify the deduplication of packages. It would at least be cleaner that way.

This does not resolve #12339 because we still need to change the pyproject and tox.ini of sphinxcontrib-* packages so that they use sphinx[dev] and not just sphinx.

@picnixz picnixz requested a review from chrisjsewell May 1, 2024 07:39
@@ -92,9 +92,13 @@ lint = [
"tomli", # for mypy (Python<=3.10)
"pytest>=6.0",
]
dev = [
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a comment, that this extra is intended for use by users of sphinx.util.testing

@chrisjsewell
Copy link
Member

Thanks @picnixz, perhaps also there should be a try/except for the imports in sphinx.util.testing, that adds a message for users to use sphinx[dev]

@picnixz
Copy link
Member Author

picnixz commented May 2, 2024

perhaps also there should be a try/except for the imports in sphinx.util.testing, that adds a message for users to use sphinx[dev]

I don't really like that. Instead, I should add some documentation for that. Otherwise, everytime a new dependency is added, a new import-guard should be added and it's an anti-pattern IMO. This also answers your first suggestion about adding a comment, and thus I'll improve the docs when I have more time.

@chrisjsewell
Copy link
Member

I don't really like that. Instead, I should add some documentation for that. Otherwise, everytime a new dependency is added, a new import-guard should be added and it's an anti-pattern IMO.

No, this is not an anti-pattern, and yes you should add it for every optional dependency; documentation will likely not help anyone that actually encounters this issue.

Literally the first package I looked for examples of handling extra dependencies, does exactly as I suggested: https://github.com/pandas-dev/pandas/blob/9110f7cdac46c69212512635a7fdc99963540c30/pandas/compat/_optional.py#L107

I you still believe it is an anti-pattern then please provide evidence to support this

@picnixz
Copy link
Member Author

picnixz commented May 2, 2024

I you still believe it is an anti-pattern then please provide evidence to support this

Well, in your example, they are doing the other way around, namely they are updating their pyproject after updating the versions. But in our case, we are centralizing everything at the level of pyproject.toml.

I would understand if we were to actually check dependencies at the code level but here we are leaving this task to package managers instead. As such I still believe it's not how we should do with our current dependency system.

In general, when you have dependencies like that, either the guard or the requirements files should be automatically generated from some specifications (it's like having a configure script which generates your Makefile accordingly).

Now, it is more common to have dev dependencies for.. development and test dependencies for testing the package itself rather than testing whatever is dependent on. And I think people would first find out about the testing plugin from the docs rather than finding out inside the code (also, people are not meant to read the pyproject.toml file since the latter is not shipped). If our docs for the testing plugin were better, then they would know that they need to install dev dependencies (also dev dependencies are common for development in general: https://github.com/search?q=path%3Apyproject.toml+%22dev+%3D+%5B%22&type=code).

Finally, using tox would technically be sufficient if we had updated our other packages to actually use dev (before test) dependencies and they would likely never found out about that issue. I really don't want to duplicate checks at the code level when it's actually the purpose of the package manager. Otherwise, we should do it for the lint dependencies in TYPE_CHECKING blocks technically.

@chrisjsewell
Copy link
Member

Finally, using tox would technically be sufficient if we had updated our other packages to actually use dev

I still feel this misses the point a little though 😅
this is nothing to do with "our" packages, I don't care about anything within https://github.com/sphinx-doc, I'm talking about users who are using sphinx.testing
You updating sphinxcontrib-* doesn't fix all my packages in https://github.com/sphinx-extensions2 etc

https://github.com/search?q=path%3Apyproject.toml+%22dev+%3D+%5B%22&type=code

But setting that aside...
the other thing to note here is that I would put good money, that all these dev extras are for internal development requirements, not user-facing ones (even though, as pep-0735 mentions, extras should really just be for user-facing dependencies).

So I feel dev is not a great name for this, especially when there is not even a comment to suggest this is for use by users.
Something like sphinx[pytest] would be better.

But look I won't hold this up, as there is not exactly practice (if only we had the wonderful features of rust 😉 https://doc.rust-lang.org/cargo/reference/features.html)

@chrisjsewell chrisjsewell requested review from chrisjsewell and removed request for chrisjsewell May 7, 2024 03:39
@picnixz
Copy link
Member Author

picnixz commented May 7, 2024

I'm always open to suggestions so I don't mind the debate!

I'm talking about users who are using sphinx.testing

Actually, the original issue was because they were testing a sphinxcontrib-* package which was installing sphinx and not sphinx[test] in their test dependency.

the other thing to note here is that I would put good money, that all these dev extras are for internal development requirements, not user-facing ones

It's probably something that has to do with the terminology in general. Since I'm coming from C/C++, devel (or dev) packages are those that are meant to be used by other packages that need to compile against our package and not just use the package "as is". Now that I looked at the results more carefully, it indeed appears that many of them are actually as internal develop, so I'll retract my argument on the "it's common to have dev".

So I feel dev is not a great name for this, especially when there is not even a comment to suggest this is for use by users. Something like sphinx[pytest] would be better

The main reason about not having a comment was more because I really don't expect people to get into the pyproject.toml file (and thus the comment wouldn't even be read). I expect those that have issues to first go to our website to see the installation instructions. As for whether sphinx[dev] or sphinx[pytest] is better, I don't mind either choice.

As for the features by rust, I'd say... it's another way to make plugins without having million of repositories outside your main source code, which in C++ usually translates into #ifdef guards (although the support is much smaller).

@chrisjsewell
Copy link
Member

chrisjsewell commented May 7, 2024

I'm always open to suggestions so I don't mind the debate!

😄 👍

because I really don't expect people to get into the pyproject.toml file

maybe just me, but I always read the pyproject.toml, and I actually added comments to the myst-parser one (well initially setup.py) specifically because people asked for it: executablebooks/MyST-Parser#139 (comment)

it's another way to make plugins without having million of repositories outside your main source code

Well thats another discussion, the main way to do this in rust is: https://doc.rust-lang.org/cargo/reference/workspaces.html (see e.g. my use in https://github.com/markdown-it-rust/markdown-it-plugins.rs/tree/main/crates and https://github.com/sphinx-extensions2/sphinx-rust)

I would love to have this in Python, and feel it could be a really good fit for sphinx.

I've tried it out personally https://github.com/chrisjsewell/pymonorepo, but its difficult with current python/pypa standards. I think main stream adoption may hopefully come when rye matures: https://rye-up.com/guide/workspaces/

Copy link
Member

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

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

I would still suggest changing the name from dev, but won't block it for this

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.

Should defusedxml be moved to the runtime dependencies?
2 participants