F401 - update documentation and deprecate ignore_init_module_imports#11436
F401 - update documentation and deprecate ignore_init_module_imports#11436
ignore_init_module_imports#11436Conversation
CodSpeed Performance ReportMerging #11436 will not alter performanceComparing Summary
|
|
|
@zanieb thoughts on how to gracefully deprecate |
|
Here's an example of a deprecated setting: ruff/crates/ruff_workspace/src/options.rs Lines 428 to 432 in a73b8c8 which has a warning at ruff/crates/ruff_workspace/src/configuration.rs Lines 423 to 430 in 6ed2482 I believe this one was deprecated in #8082 if that's helpful. I'm actually not entirely sure why we define a message for the deprecation and manually include a separate warning. Perhaps because it was renamed? I'd have to play with this a bit to find the right usage. I think we should still respect the setting on stable but you can ignore it in preview. In both cases, we should display a warning. In the future, we can make using the setting an error during configuration loading. |
…as message to use that instead of the binding name
…mports=false: emit unsafe fixes to remove unused imports even in __init__.py
…emove unused imports even in __init__.py
| let in_init = checker.path().ends_with("__init__.py"); | ||
| let fix_init = checker.settings.preview.is_enabled(); | ||
| let fix_init = !checker.settings.ignore_init_module_imports; | ||
| let preview_mode = checker.settings.preview.is_enabled(); |
There was a problem hiding this comment.
These three lines look fishy because I'd originally replaced this
let fix_init = !checker.settings.ignore_init_module_imports;with
let fix_init = checker.settings.preview.is_enabled();which was perhaps lazy. In this PR I'm restoring the old behavior, and so gave checker.settings.preview.is_enabled(); its own name.
| Init { | ||
| first_party: bool, | ||
| dunder_all_count: usize, | ||
| ignore_init_module_imports: bool, |
There was a problem hiding this comment.
This field is required to make fix titles follow the "old" behavior in __init__.py files. When we finally remove the option ignore_init_module_imports then this field ignore_init_module_imports can be deleted too.
| }); | ||
|
|
||
| // generate fixes that are shared across bindings in the statement | ||
| let (fix_remove, fix_reexport) = if (!in_init || fix_init) && !in_except_handler { |
There was a problem hiding this comment.
This section looks like a large change but isn't. I only added preview_mode to the disjunction. Turn on -w to see the diff w/o whitespace changes.
| | ^^^^ F401 | ||
| | | ||
| = help: Use an explicit re-export: `bees as bees` | ||
| = help: Use an explicit re-export: `renamed as renamed` |
There was a problem hiding this comment.
This is the only change in to an existing snapshot in the whole diff. It was a bug where the fix title contained the binding, instead of the module.
|
Deferring to @zanieb, feel free to merge when approved! |
| Fixes to remove unused imports are safe, except in `__init__.py` files. | ||
|
|
||
| Applying fixes to `__init__.py` files is currently in preview. The fix offered depends on the | ||
| type of the unused import. Ruff will suggest a safe fix to export first-party imports with | ||
| either a redundant alias or, if already present in the file, an `__all__` entry. If multiple | ||
| `__all__` declarations are present, Ruff will not offer a fix. Ruff will suggest an unsafe fix | ||
| to remove third-party and standard library imports -- the fix is unsafe because the module's | ||
| interface changes. |
There was a problem hiding this comment.
I think it's good that ruff has different handling of first vs third party imports, but changing behavior based on the name of the module (containing the import) I find unintuitive and I argue is unpythonic.
By that, I mean: if I have mod/__init__.py, is the fix any more or less safe than if I instead named it mod.py? No — the __init__.py is just an implementation detail and in either case, the module's interface changes with any import changes.
It's not a universally-accepted convention that __init__.py should store your entire public API.
For example, it's hinders import times for users of a large package, if the entire public API is placed in __init__.py, because then any import of a submodule will trigger an import __init__.py and thus an import of every module, if everything is reexported.
At most, we have the typing docs (Typing documentation: interface conventions) because there's nothing I can find on python.org that discusses this. But given the typing docs guidance, I would find it slightly easier to teach that:
- third party imports: always safe to remove
- first party imports: unsafe to remove if contained within the same package (similar to relative import convention from typing spec). Safe otherwise.
There was a problem hiding this comment.
Also for mono-repos, there may be dozens or hundreds of first party top-level packages. Therefore this rule should base its decisions on whether an import is intra-package (e.g. a relative import) rather than first vs third party packages.
There was a problem hiding this comment.
Thanks for your feedback @Hnasar — would you mind opening a new issue to discuss this preview behavior? I don't think this pull request is a great place for it since it's just updating the docs.
Summary
__all__#11314ignore_init_module_importsignore_init_module_importsis set totrue(default) there are no__init_.pyfixes (but we get nice fix titles!).ignore_init_module_importsis set tofalsethere are unsafe__init__.pyfixes to remove unused imports.ignore_init_module_imports.import foo as barwould recommend reexportingbar as bar. It now says to reexportfoo as foo. (In this case we don't issue a fix, fwiw; it was just a fix title bug.)Test plan
Added new fixture tests that reuse the existing fixtures for
__init__.pyfiles. Each of the three situations listed above has fixture tests. The F401 "stable" tests cover:The F401 "deprecated option" tests cover:
These complement existing "preview" tests that show the new behavior which recommends fixes in
__init__.pyaccording to whether the import is 1st party and other circumstances (for more on that behavior see: #11314).