-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Guard Type Hint Imports Used Only Inside Annotations #12488
Guard Type Hint Imports Used Only Inside Annotations #12488
Conversation
86b1427
to
39a910a
Compare
I'm -0 on this. Given that Without some specific benefit that we can't get any other way (and I consider "not annotating a problem function" to be a valid "other way") I don't think we should do this. But honestly, I find it difficult to care about type annotations, so I'm not going any further than -0. |
During type checking phase there is no difference between During runtime, for most usecases, there is a bigger difference between PEP 649 and not having Therefore I've moved most by codebase to include
I don't 100% follow this, but Pip already has to do a lot of With these rules most choices would have become obvious, if it's an annotation only then put it in a type checking block that's import guarded. I then don't have to worry about if the type was introducted in Python 3.8 or if I should create some fake runtime object (which exists in places in the Pip codebase right now).
This is not a hill I'm going to die on, I just found it useful in my own codebases. I've outlined the exact steps I did to create this PR, it can easily be revisited if my predictions of supporting Python versions across PEP 649 and non-PEP 649 come true, by myself or someone else. |
One additional benefit, which this commit highlited: 39a910a Is it prevents importing objects for runtime uses from a module that was only importing that object for type hint annotations, which seems like a strong code smell to me. |
Closing this PR due to expressed non-intesterst from Pip maintainers, and as it affects so many files it will inevetibly code rot and it should not be manually fixed but rather the automated steps I described should be reapplied on clean codebase: #12488 (comment) As a quick summary if anyone wants to revist this again, the benefits are:
Further cleans ups could also be acheived:
|
I'm submitting this change because I recently implemented it in my company's codebase and found it beneficial. I understand though if Pip maintainers choose not to adopt it.
This PR utilizes ruff to introduce
from __future__ import annotations
as a mandatory import. This allows all flake8-type-checking rules to be enabled, via ruff, to relocate imports used solely as annotations into a type-checking import guard block.Motivation:
The commits are as follows:
from __future__ import annotations
ruff check . --fix
ruff check . --select TCH001 --fix --unsafe-fixes
ruff check . --select TCH002 --fix --unsafe-fixes
ruff check . --select TCH003 --fix --unsafe-fixes
No manual touch-ups were applied in these commits. However, I've observed that some "else" blocks in "if TYPE_CHECKING" can be removed after the above commits.
Finally, note that
from __future__ import annotations
can be removed when Pip only needs to support Python versions with PEP 649, which would not be before October 2029.