Remove F401 fix for __init__ imports by default and allow opt-in to unsafe fix
#10365
Conversation
zanieb
changed the title
ignore-init-module-imports and make default behaviorF401 fix for __init__ imports by default and allow opt-in to unsafe fix
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| F401 | 104 | 52 | 52 | 0 | 0 |
Linter (preview)
ℹ️ ecosystem check detected linter changes. (+52 -52 violations, +0 -0 fixes in 2 projects; 41 projects unchanged)
PostHog/HouseWatch (+2 -2 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview
- housewatch/models/__init__.py:2:21: F401 [*] `.backup.ScheduledBackup` imported but unused + housewatch/models/__init__.py:2:21: F401 `.backup.ScheduledBackup` imported but unused; consider removing, adding to `__all__`, or using a redundant alias - housewatch/models/__init__.py:2:38: F401 [*] `.backup.ScheduledBackupRun` imported but unused + housewatch/models/__init__.py:2:38: F401 `.backup.ScheduledBackupRun` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
latchbio/latch (+50 -50 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview
- latch/__init__.py:10:5: F401 [*] `latch.functions.operators.group_tuple` imported but unused + latch/__init__.py:10:5: F401 `latch.functions.operators.group_tuple` imported but unused; consider removing, adding to `__all__`, or using a redundant alias - latch/__init__.py:11:5: F401 [*] `latch.functions.operators.inner_join` imported but unused + latch/__init__.py:11:5: F401 `latch.functions.operators.inner_join` imported but unused; consider removing, adding to `__all__`, or using a redundant alias - latch/__init__.py:12:5: F401 [*] `latch.functions.operators.latch_filter` imported but unused + latch/__init__.py:12:5: F401 `latch.functions.operators.latch_filter` imported but unused; consider removing, adding to `__all__`, or using a redundant alias - latch/__init__.py:13:5: F401 [*] `latch.functions.operators.left_join` imported but unused + latch/__init__.py:13:5: F401 `latch.functions.operators.left_join` imported but unused; consider removing, adding to `__all__`, or using a redundant alias - latch/__init__.py:14:5: F401 [*] `latch.functions.operators.outer_join` imported but unused + latch/__init__.py:14:5: F401 `latch.functions.operators.outer_join` imported but unused; consider removing, adding to `__all__`, or using a redundant alias - latch/__init__.py:15:5: F401 [*] `latch.functions.operators.right_join` imported but unused + latch/__init__.py:15:5: F401 `latch.functions.operators.right_join` imported but unused; consider removing, adding to `__all__`, or using a redundant alias - latch/__init__.py:17:41: F401 [*] `latch.resources.conditional.create_conditional_section` imported but unused + latch/__init__.py:17:41: F401 `latch.resources.conditional.create_conditional_section` imported but unused; consider removing, adding to `__all__`, or using a redundant alias - latch/__init__.py:18:39: F401 [*] `latch.resources.map_tasks.map_task` imported but unused + latch/__init__.py:18:39: F401 `latch.resources.map_tasks.map_task` imported but unused; consider removing, adding to `__all__`, or using a redundant alias - latch/__init__.py:19:48: F401 [*] `latch.resources.reference_workflow.workflow_reference` imported but unused + latch/__init__.py:19:48: F401 `latch.resources.reference_workflow.workflow_reference` imported but unused; consider removing, adding to `__all__`, or using a redundant alias - latch/__init__.py:21:5: F401 [*] `latch.resources.tasks.custom_task` imported but unused + latch/__init__.py:21:5: F401 `latch.resources.tasks.custom_task` imported but unused; consider removing, adding to `__all__`, or using a redundant alias - latch/__init__.py:22:5: F401 [*] `latch.resources.tasks.custom_memory_optimized_task` imported but unused + latch/__init__.py:22:5: F401 `latch.resources.tasks.custom_memory_optimized_task` imported but unused; consider removing, adding to `__all__`, or using a redundant alias - latch/__init__.py:23:5: F401 [*] `latch.resources.tasks.large_gpu_task` imported but unused + latch/__init__.py:23:5: F401 `latch.resources.tasks.large_gpu_task` imported but unused; consider removing, adding to `__all__`, or using a redundant alias - latch/__init__.py:24:5: F401 [*] `latch.resources.tasks.large_task` imported but unused + latch/__init__.py:24:5: F401 `latch.resources.tasks.large_task` imported but unused; consider removing, adding to `__all__`, or using a redundant alias - latch/__init__.py:25:5: F401 [*] `latch.resources.tasks.medium_task` imported but unused + latch/__init__.py:25:5: F401 `latch.resources.tasks.medium_task` imported but unused; consider removing, adding to `__all__`, or using a redundant alias - latch/__init__.py:26:5: F401 [*] `latch.resources.tasks.small_gpu_task` imported but unused + latch/__init__.py:26:5: F401 `latch.resources.tasks.small_gpu_task` imported but unused; consider removing, adding to `__all__`, or using a redundant alias - latch/__init__.py:27:5: F401 [*] `latch.resources.tasks.small_task` imported but unused + latch/__init__.py:27:5: F401 `latch.resources.tasks.small_task` imported but unused; consider removing, adding to `__all__`, or using a redundant alias - latch/__init__.py:29:38: F401 [*] `latch.resources.workflow.workflow` imported but unused + latch/__init__.py:29:38: F401 `latch.resources.workflow.workflow` imported but unused; consider removing, adding to `__all__`, or using a redundant alias - latch/__init__.py:7:38: F401 [*] `latch.functions.messages.message` imported but unused + latch/__init__.py:7:38: F401 `latch.functions.messages.message` imported but unused; consider removing, adding to `__all__`, or using a redundant alias - latch/__init__.py:9:5: F401 [*] `latch.functions.operators.combine` imported but unused + latch/__init__.py:9:5: F401 `latch.functions.operators.combine` imported but unused; consider removing, adding to `__all__`, or using a redundant alias - latch/types/__init__.py:10:5: F401 [*] `latch.types.metadata.LatchMetadata` imported but unused + latch/types/__init__.py:10:5: F401 `latch.types.metadata.LatchMetadata` imported but unused; consider removing, adding to `__all__`, or using a redundant alias - latch/types/__init__.py:11:5: F401 [*] `latch.types.metadata.LatchParameter` imported but unused + latch/types/__init__.py:11:5: F401 `latch.types.metadata.LatchParameter` imported but unused; consider removing, adding to `__all__`, or using a redundant alias - latch/types/__init__.py:12:5: F401 [*] `latch.types.metadata.LatchRule` imported but unused + latch/types/__init__.py:12:5: F401 `latch.types.metadata.LatchRule` imported but unused; consider removing, adding to `__all__`, or using a redundant alias - latch/types/__init__.py:13:5: F401 [*] `latch.types.metadata.Params` imported but unused + latch/types/__init__.py:13:5: F401 `latch.types.metadata.Params` imported but unused; consider removing, adding to `__all__`, or using a redundant alias - latch/types/__init__.py:14:5: F401 [*] `latch.types.metadata.Section` imported but unused + latch/types/__init__.py:14:5: F401 `latch.types.metadata.Section` imported but unused; consider removing, adding to `__all__`, or using a redundant alias ... 52 additional changes omitted for project
Changes by rule (1 rules affected)
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| F401 | 104 | 52 | 52 | 0 | 0 |
|
What's the motivation for retaining the diagnostic but not offering the fix? I'm generally not a fan of that pattern -- it ends up being somewhat user-unfriendly. |
|
@charliermarsh I think the fix is more than unsafe, I think it's wrong. I'd prefer to offer a different fix: inclusion in |
|
Should we be raising the diagnostic at all then? |
|
@charliermarsh I think having a diagnostic without an automated fix is totally normal? |
|
To be clear: I think in the future we should remove the setting entirely and always offer a safe fix in |
|
That's obviously a different situation since we were introducing more diagnostics, but it made me generally even more cautious of the idea of gating fixes from users. In this case, I'm mostly just wondering if we should flag this at all, since we have no idea if the import is unused. |
|
I think that's a fair point. I think there's a great deal of value to this diagnostic but honestly it seems like it should be under a different rule code. I think what I have here is still the best past forward for parity with the existing tool (i.e. PyCQA/pyflakes#471). It moves us towards better default behavior while allowing users to fully recover the existing behavior. I think we have a reasonable path towards additional improvements, I'm just not doing them here because I'm trying to do a small amount of work to close old pull requests and issues. |
|
What if in I think it would be nice to carry on removing stdlib imports in particular, since they're really common, should be relatively easy to identify, and you're very rarely going to actually be trying to reexport a stdlib thing from some third-party package. |
|
@AlexWaygood I'm all for that. Could we introduce it in a follow-up or do you think it's blocking? |
| @@ -90,7 +95,7 @@ impl Violation for UnusedImport { | |||
| } | |||
| Some(UnusedImportContext::Init) => { | |||
| format!( | |||
| "`{name}` imported but unused; consider adding to `__all__` or using a redundant alias" | |||
| "`{name}` imported but unused; consider removing, adding to `__all__`, or using a redundant alias" | |||
There was a problem hiding this comment.
I'd usually refer to this as an "explicit reexport" rather than "redundant alias", FWIW, but "redundant alias" is probably clearer for people unfamiliar with the idea
There was a problem hiding this comment.
"redundant alias" is the term used in Pyright which is usually my reference for this
There was a problem hiding this comment.
@AlexWaygood I'm all for that. Could we introduce it in a follow-up or do you think it's blocking?
I think it can be done as a followup (but let's open an issue to track it, since I agree with @charliermarsh that we should do our best to provide autofixes for errors whenever possible).
This LGTM; I agree removing imports automatically from __init__.py files seems too risky for it to be done as a safe fix. Left some minor comments above.
|
To clarify: does that mean we wouldn't flag F401 for non-standard library imports in |
I think the ideal behavior is:
We could consider not doing (1) for a subset until we have the fixes e.g. (3) but I would rather have the diagnostic without a fix than no diagnostic at all. |
I was suggesting the latter. I think if you want to re-export in an |
|
Okay, seems reasonable to me. |
Unless we can reliably distinguish between local imports and third-party dependencies, I'd honestly probably just not fix non-stdlib imports at all in an |
Co-authored-by: Alex Waygood <[email protected]>
Co-authored-by: Alex Waygood <[email protected]>
I think we have reasonable support for this |
Thanks! |
|
FWIW, I've had both |
|
@tylerlaprade Yeah I think so. If you're a library author and you want to define an API, you should properly export types. If you don't, |
|
@zanieb note that this is not just a pyright convention, but a Python typing convention in general: Looking forward to the two follow up improvements though! F401 is one of my faves 😄 That said, after updating, we were surprised to see this fix wasn't being applied, so we reverted to the pre-3.3 behavior with: [tool.ruff.lint]
# Allow F401 to be honored in __init__.py files
ignore-init-module-imports = false
# Allow applying these unsafe fixes without the `--unsafe-fixes` flag
extend-safe-fixes = ["F401"] |
|
Thanks for the feedback! Glad to see that's documented clearly elsewhere, pyright was just my goto resource for a while :) |
Re-implementation of #5845 but instead of deprecating the option I toggle the default. Now users can opt-in via the setting which will give them an unsafe fix to remove the import. Otherwise, we raise violations but do not offer a fix. The setting is a bit of a misnomer in either case, maybe we'll want to remove it still someday.
As discussed there, I think the safe fix should be to import it as an alias. I'm not sure. We need support for offering multiple fixes for ideal behavior though? I think we should remove the fix entirely and consider it separately.
Closes #5697
Supersedes #5845