analyze: Add option to skip over imports in TYPE_CHECKING blocks #21472
Conversation
|
| Pipeline fails cause Clippy is complaining there are too many bools in |
MichaReiser changed the title TYPE_CHECKING imports in ruff analyze graphanalyze: Add option to skip over imports in TYPE_CHECKING blocks 
MichaReiser left a comment
MichaReiser left a comment There was a problem hiding this comment.
Thank you. I left a few inline comments with a few suggestions
crates/ruff/src/args.rs Outdated
| python: Option<PathBuf>, | ||
| /// Exclude imports that are only used for type checking (i.e., imports within `if TYPE_CHECKING:` blocks) | ||
| #[arg(long)] | ||
| exclude_type_checking_imports: bool, |
There was a problem hiding this comment.
I think I'd name this type_checking_only_imports or just type_checking_imports where true includes them and false skips them (should default to true).
We should also add this as a configuration optiont to
ruff/crates/ruff_workspace/src/options.rs
Line 3816 in 6e4b15b
There was a problem hiding this comment.
I've done both.
Adding it as a config option ended up being a bigger change than I expected. I tried to follow how it was done for detect-string-imports. Please let me know if that is good.
There was a problem hiding this comment.
I also just noticed that preview and string_imports config options are not boolean, but instead "enabled" or "disabled". Should I model it that way instead?
There was a problem hiding this comment.
Yeah, it requires a lot of boilerplate code.
t preview and string_imports config options are not boolean, but instead "enabled" or "disabled". Should I model it that way instead?
I think a boolean is fine.
Commit adds functionality to enabled or disable type checking imports, either as a cli arg or a config file
gauthsvenkat force-pushed the feat/ruff-graph-type-checking-imports branch from 486ab6b to eda7e8b Compare gauthsvenkat force-pushed the feat/ruff-graph-type-checking-imports branch from b681eac to 48a3e66 Compare 
| if self.type_checking_imports || !is_type_checking_condition(test) { | ||
| self.visit_body(body); |
There was a problem hiding this comment.
We could consider handling
if !TYPE_CHECKING: ... else: ...and
if condition: ... elif TYPE_CHECKING: ...but we can do this as a separate PR
* origin/main: (26 commits) Mention `force-exclude` in "Configuration > Python file discovery" (#21500) Avoid syntax error when formatting attribute expressions with outer parentheses, parenthesized value, and trailing comment on value (#20418) [ty] suppress invalid suggestions in import statements (#21484) Limit `eglot-format` hook to eglot-managed Python buffers (#21459) Adjust own-line comment placement between branches (#21185) [ty] Subscript assignment diagnostics follow-up (#21452) [ty] Inlay hint call argument location (#20349) [ty] Use `CompactStr` for `StringLiteralType` (#21497) Update CodSpeedHQ/action action to v4.3.4 (#21488) Update salsa digest to a885bb4 (#21486) Update dependency ruff to v0.14.5 (#21489) Update astral-sh/setup-uv action to v7.1.3 (#21487) Update Rust crate get-size2 to v0.7.2 (#21490) Update Rust crate indicatif to v0.18.3 (#21491) Update Rust crate quick-junit to v0.5.2 (#21492) Update taiki-e/install-action action to v2.62.52 (#21493) Fix analyze graph tests on windows (#21481) `analyze`: Add option to skip over imports in `TYPE_CHECKING` blocks (#21472) [ty] Dataclasses: `__hash__` semantics and `unsafe_hash` (#21470) [ty] Dataclass transform: complete set of parameters (#21474) ...
3 participants
Summary
ruff analyze graphnaively detects all imports. This MR adds a feature whereTYPE_CHECKINGimports can be detected and optionally excluded (using a CLI flag).Closes: #20736
Main changes
CollectedImportis now astructinstead of anenum. This was done so that a newtype_checkingfield could be added.Stmt::Import{,From}into helper functionscollect_import{,_from}. Done so they can be reused inStmt::Ifto detect type checking imports.is_type_checking_condition. This can detect very simple type-checking conditions, but, practically, it would cover a lot of the cases.Test Plan
--exclude-type-checking-importsflag is set.