-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Move static-checkers-only dependencies into their dedicated extras #4586
Move static-checkers-only dependencies into their dedicated extras #4586
Conversation
7a26b0d
to
e155cd9
Compare
pyproject.toml
Outdated
# but they're still needed for type-checking since we import them directly | ||
"tomli", | ||
"importlib_resources", | ||
"importlib_metadata", |
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 wonder, can we have these conditional on the Python version?
I think it would be interesting from the testing perspective to see how the test suite behaves when these packages are not installed.
In the _importlib.py
file we have if sys.version_info ...
protecting the imports, do you think the typechecking would be smart enough to deal with that?
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.
Yes, type checkers should understand simple version and platform checks. That would allow removing some entries from here.
However, note that currently mypy only tests for Python 3.8: #4352
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.
That is an interesting point.
What happens if we run mypy
(via pytest-mypy
) with the Python 3.12 executable, but in mypy.ini
we have python_version = 3.8
? Will mypy
simulate that the file is running under 3.8
even for things like sys.version_info
?
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.
Exactly. And it'll take into account modules that are present (or unavailable) in stdlib as specified by typeshed. You could run all your version and platform checks from the same Python executable (as long as all your packages are present in your environment and you don't use new syntax that required AST changes)
Pyright (#4192) and Ruff have an advantage where they're not restricted by the executed Python version (since they don't run in Python). So AST changes are irrelevant to them (hence they haven't dropped Python 3.7 support)
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 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.
We should be able to remove them. I'm on it 😃 (and will let you know if anything else comes ups)
61c502d
to
759722e
Compare
6405fa9
to
efeaa67
Compare
if sys.version_info >= (3, 11): | ||
import tomllib | ||
else: # pragma: no cover | ||
import tomli as tomllib | ||
|
||
archive = Archive(sdist_file) | ||
info = toml.loads(_read_pyproject(archive)) | ||
info = tomllib.loads(_read_pyproject(archive)) |
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 think technically this is not currently needed to pass CI because our mypy configs cause this method to not be type-checked yet.
But it would be needed eventually or with #4192
6cd1a95
to
93de877
Compare
93de877
to
27c258f
Compare
Summary of changes
0588af0 added extras specific for type-checking and "checkers" (lint+format). I think other dependencies that are only specified for these tests should also be moved there ?
Pull Request Checklist
newsfragments/
. (not user facing)(See documentation for details)