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

[pyflakes] make F401 autofix "suggested" in __init__.py files #5845

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion crates/ruff/src/rules/pyflakes/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,11 @@ mod tests {
fn init() -> Result<()> {
let diagnostics = test_path(
Path::new("pyflakes/__init__.py"),
&Settings::for_rules(vec![Rule::UndefinedName, Rule::UndefinedExport]),
&Settings::for_rules(vec![
Rule::UndefinedName,
Rule::UndefinedExport,
Rule::UnusedImport,
]),
)?;
assert_messages!(diagnostics);
Ok(())
Expand Down
20 changes: 14 additions & 6 deletions crates/ruff/src/rules/pyflakes/rules/unused_import.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,7 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut
}
}

let in_init =
checker.settings.ignore_init_module_imports && checker.path().ends_with("__init__.py");
let in_init = checker.path().ends_with("__init__.py");

// Generate a diagnostic for every import, but share a fix across all imports within the same
// statement (excluding those that are ignored).
Expand All @@ -152,8 +151,8 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut
exceptions.intersects(Exceptions::MODULE_NOT_FOUND_ERROR | Exceptions::IMPORT_ERROR);
let multiple = imports.len() > 1;

let fix = if !in_init && !in_except_handler && checker.patch(Rule::UnusedImport) {
fix_imports(checker, stmt_id, &imports).ok()
let fix = if !in_except_handler && checker.patch(Rule::UnusedImport) {
fix_imports(checker, stmt_id, &imports, in_init).ok()
} else {
None
};
Expand Down Expand Up @@ -224,7 +223,12 @@ struct Import<'a> {
}

/// Generate a [`Fix`] to remove unused imports from a statement.
fn fix_imports(checker: &Checker, stmt_id: NodeId, imports: &[Import]) -> Result<Fix> {
fn fix_imports(
checker: &Checker,
stmt_id: NodeId,
imports: &[Import],
in_init: bool,
) -> Result<Fix> {
let stmt = checker.semantic().stmts[stmt_id];
let parent = checker.semantic().stmts.parent(stmt);
let edit = autofix::edits::remove_unused_imports(
Expand All @@ -237,5 +241,9 @@ fn fix_imports(checker: &Checker, stmt_id: NodeId, imports: &[Import]) -> Result
checker.stylist,
checker.indexer,
)?;
Ok(Fix::automatic(edit).isolate(checker.isolation(parent)))
if in_init {
Ok(Fix::suggested(edit).isolate(checker.isolation(parent)))
} else {
Ok(Fix::automatic(edit).isolate(checker.isolation(parent)))
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,19 @@
---
source: crates/ruff/src/rules/pyflakes/mod.rs
---
__init__.py:1:8: F401 [*] `os` imported but unused; consider adding to `__all__` or using a redundant alias
|
1 | import os
| ^^ F401
2 |
3 | print(__path__)
|
= help: Remove unused import: `os`
Comment on lines +4 to +11
Copy link
Member

Choose a reason for hiding this comment

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

It seems like we should suggest import os as os instead

Copy link
Member

Choose a reason for hiding this comment

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

We do include this in the message ("or using a redundant alias"), unsure what the right suggested fix is...

Copy link
Member

@zanieb zanieb Jul 17, 2023

Choose a reason for hiding this comment

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

We don't mention removing it in the message though — perhaps if we say "remove it or consider adding..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so maybe the suggested fix should be

- import os
+ import os as os

?

Copy link
Member

Choose a reason for hiding this comment

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

(Sorry to come back after so long) It's tough because we really want to offer two different fixes here. I guess we should leave the fix as-is, but refine the diagnostic message.


ℹ Suggested fix
1 |-import os
2 1 |
3 2 | print(__path__)
4 3 |


2 changes: 2 additions & 0 deletions crates/ruff/src/settings/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,8 @@ pub struct Options {
/// imports will still be flagged, but with a dedicated message suggesting
/// that the import is either added to the module's `__all__` symbol, or
/// re-exported with a redundant alias (e.g., `import os as os`).
///
/// This option has been **deprecated** and its effect is now the default behavior.
pub ignore_init_module_imports: Option<bool>,
#[option(
default = r#"["*.py", "*.pyi", "**/pyproject.toml"]"#,
Expand Down
2 changes: 1 addition & 1 deletion ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.