-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[flake8-type-checking
, pyupgrade
, ruff
] Add from __future__ import annotations
when it would allow new fixes (TC001
, TC002
, TC003
, UP037
, RUF013
)
#19100
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
Conversation
|
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.
Looks pretty good to me, but I think we should test the interaction with quote-annotations
more carefully.
crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs
Outdated
Show resolved
Hide resolved
Do we need to add the same check to other rules as well (UP?). |
Thanks @Daverball, your suspicions were true. I wasn't handling the quoting setting properly. I resurrected some of the ideas from #18919 but with four values, as you suggested there ( I think this looks a lot nicer (modulo the somewhat awkward names I picked) and handles both settings being enabled, as the updated tests show. I'll extend this to the UP rules tomorrow, but hopefully this is a solid draft of the TC rules. |
aa2332c
to
44ae933
Compare
428cc97
to
2a6bd8c
Compare
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.
(Micha asked me to take a quick look, but I haven't done an exhaustive review. It all basically LGTM!)
Co-authored-by: Alex Waygood <[email protected]>
it's not clear from the call site that we don't need to `continue` to avoid extra work if the setting is disabled, so mention it here
Ah, I see what you mean. My draft fix added the import but didn't unquote the annotation, which is why I thought it didn't make much sense. If we add the import and then unquote the annotation, this would be a nice fix. I added this and the other rules we mentioned to #19359. Let me know if I missed any! |
enable PreviewMode in the new tests
I'll do another review tomorrow. Could you rebase the PR before then? |
Yep, I was merging main and getting ready to push right now! |
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.
This is great. Thank you for pushing this forward
|| (settings.quote_annotations && reference.in_runtime_evaluated_annotation()))) | ||
use crate::settings::LinterSettings; | ||
|
||
/// Represents the kind of an existing or potential typing-only annotation. |
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.
Let's add a comment explaining that the variants need to be ordered by their precedence. It's sort of important
/// | ||
/// For example, `TC001`, `TC002`, and `TC003` can move more imports into `TYPE_CHECKING` blocks | ||
/// if `__future__` annotations are enabled. | ||
#[option( |
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 we should mention that this option is preview only for now
update the `combine` docs with the new names
flake8-type-checking
] Add from __future__ import annotations
to allow a fixflake8-type-checking
, pyupgrade
, ruff
] Add from __future__ import annotations
when it would allow new fixes (TC001
, TC002
, TC003
, UP037
, RUF013
)
Summary
This is a second attempt at addressing #18502 instead of reusing
FA100
(#18919).This PR:
lint.allow-importing-future-annotations
option__future__
import when it would triggerTC001
,TC002
, orTC003
|
union syntax before 3.10 in implicit-optional (RUF013)I started adding a fix for runtime-string-union (TC010) too, as mentioned in my previous comment, but some of the existing tests already imported
from __future__ import annotations
, so I think we intentionally flag these cases for the user to inspect. Adding the import is a fix but probably not the best one.Test Plan
Existing
TC
tests, new copies of them with the option enabled, and new tests based on ideas in #18919 (comment) and the following thread. For UP037 and RUF013, the new tests are also copies of the existing tests, with the new option enabled. The easiest way to review them is probably by their diffs from the existing snapshots:UP037
UP037_0.py
andUP037_2.pyi
have no diffs. The diff forUP037_1.py
is below. It correctly unquotes an annotation in module scope that would otherwise be invalid.UP037_1.py
RUF013
The diffs here are mostly just the imports because the original snaps were on 3.13. So we're getting the same fixes now on 3.9.
RUF013_0.py
RUF013_1.py
RUF013_3.py
RUF013_4.py
Future work
This PR does not touch UP006, UP007, or UP045, which are currently coupled to FA100. If this new approach turns out well, we may eventually want to deprecate FA100 and add a
__future__
import in those rules' fixes too.