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

F401 - update documentation and deprecate ignore_init_module_imports #11436

Merged
merged 20 commits into from
May 21, 2024

Conversation

plredmond
Copy link
Contributor

@plredmond plredmond commented May 15, 2024

Summary

  • Update documentation for F401 following recent PRs
  • Deprecate ignore_init_module_imports
    • Add a deprecation pragma to the option and a "warn user once" message when the option is used.
  • Restore the old behavior for stable (non-preview) mode:
    • When ignore_init_module_imports is set to true (default) there are no __init_.py fixes (but we get nice fix titles!).
    • When ignore_init_module_imports is set to false there are unsafe __init__.py fixes to remove unused imports.
    • When preview mode is enabled, it overrides ignore_init_module_imports.
  • Fixed a bug in fix titles where import foo as bar would recommend reexporting bar as bar. It now says to reexport foo 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__.py files. Each of the three situations listed above has fixture tests. The F401 "stable" tests cover:

  • When ignore_init_module_imports is set to true (default) there are no __init_.py fixes (but we get nice fix titles!).

The F401 "deprecated option" tests cover:

  • When ignore_init_module_imports is set to false there are unsafe __init__.py fixes to remove unused imports.

These complement existing "preview" tests that show the new behavior which recommends fixes in __init__.py according to whether the import is 1st party and other circumstances (for more on that behavior see: #11314).

Copy link

codspeed-hq bot commented May 15, 2024

CodSpeed Performance Report

Merging #11436 will not alter performance

Comparing ruff.all (6028415) with main (403f0dc)

Summary

✅ 30 untouched benchmarks

Copy link
Contributor

github-actions bot commented May 15, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@plredmond
Copy link
Contributor Author

@zanieb thoughts on how to gracefully deprecate lint.ignore_init_module_imports? I can just grep for it and remove it, but that seems like it would break people who have the option in their config files.

@zanieb
Copy link
Member

zanieb commented May 18, 2024

Here's an example of a deprecated setting:

#[deprecated(
since = "0.1.2",
note = "The `tab-size` option has been renamed to `indent-width` to emphasize that it configures the indentation used by the formatter as well as the tab width. Please update your configuration to use `indent-width = <value>` instead."
)]
pub tab_size: Option<IndentWidth>,

which has a warning at

#[allow(deprecated)]
let indent_width = {
if options.tab_size.is_some() {
warn_user_once!("The `tab-size` option has been renamed to `indent-width` to emphasize that it configures the indentation used by the formatter as well as the tab width. Please update your configuration to use `indent-width = <value>` instead.");
}
options.indent_width.or(options.tab_size)
};

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.

@@ -244,7 +268,8 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut
}

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();
Copy link
Contributor Author

@plredmond plredmond May 20, 2024

Choose a reason for hiding this comment

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

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.

@@ -24,6 +24,7 @@ enum UnusedImportContext {
Init {
first_party: bool,
dunder_all_count: usize,
ignore_init_module_imports: bool,
Copy link
Contributor Author

@plredmond plredmond May 20, 2024

Choose a reason for hiding this comment

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

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 {
Copy link
Contributor Author

@plredmond plredmond May 20, 2024

Choose a reason for hiding this comment

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

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.

@@ -39,4 +39,4 @@ __init__.py:36:26: F401 `.renamed` imported but unused; consider removing, addin
36 | from . import renamed as bees # F401: no fix
| ^^^^ F401
|
= help: Use an explicit re-export: `bees as bees`
= help: Use an explicit re-export: `renamed as renamed`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@plredmond plredmond marked this pull request as ready for review May 20, 2024 23:29
@charliermarsh
Copy link
Member

Deferring to @zanieb, feel free to merge when approved!

Comment on lines +49 to +56
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

@plredmond plredmond merged commit 7225732 into main May 21, 2024
19 checks passed
@plredmond plredmond deleted the ruff.__all__ branch May 21, 2024 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants