-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[pyflakes
] make F401 autofix "suggested" in __init__.py
files
#5845
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing! I just have one comment :)
__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` |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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..."
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
PR Check ResultsEcosystemℹ️ ecosystem check detected changes. (+2, -2, 0 error(s)) setuptools (+2, -2)
- pkg_resources/_vendor/jaraco/text/__init__.py:9:58: F401 [*] `pkg_resources.extern.importlib_resources.files` imported but unused + pkg_resources/_vendor/jaraco/text/__init__.py:9:58: F401 [*] `pkg_resources.extern.importlib_resources.files` imported but unused; consider adding to `__all__` or using a redundant alias - setuptools/_vendor/jaraco/text/__init__.py:9:55: F401 [*] `setuptools.extern.importlib_resources.files` imported but unused + setuptools/_vendor/jaraco/text/__init__.py:9:55: F401 [*] `setuptools.extern.importlib_resources.files` imported but unused; consider adding to `__all__` or using a redundant alias
|
I'm still up for removing this! |
…to unsafe fix (#10365) 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 --------- Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Implemented in #10365 Thanks for contributing this was a helpful reference! |
Summary
Make the F401 (
unused-import
) autofix "suggested", instead of "automatic".Related:
__init__
files #5697.Test Plan
unused-import
rule in the existing pyflakes test for__init__.py
files and validate the snapshot