gh-120417: Add #noqa to used imports in the stdlib by vstinner · Pull Request #120421 · python/cpython

vstinner · 2024-06-12T16:26:02Z

Tools such as ruff can ignore "imported but unused" warnings if a line ends with "# noqa". It avoids the temptation to remove an import which is used effectively.

Issue: Remove unused imports from the stdlib #120417

Tools such as ruff can ignore "imported but unused" warnings if a line ends with "# noqa". It avoids the temptation to remove an import which is used effectively.

AlexWaygood

Thanks! Looks much better now, a couple of questions though:

AlexWaygood · 2024-06-12T19:14:53Z

 try:
- from _collections import _deque_iterator
+ from _collections import _deque_iterator # noqa
 except ImportError:
 pass


test_collections passes for me if I remove this import. Does it need to stay? It was added in 52f96d3

@erlend-aasland @kumaraditya303: Do you know/recall why this symbol is exposed?

Oh, test_deque fails if the import is removed. I expected deque to be tested as part of test_collections, but apparently it has its own test file. Could you add a comment that it is (apparently) required to expose it in the collections module in order for deque iterators to be pickled?
====================================================================== ERROR: test_iterator_pickle (test.test_deque.TestBasic.test_iterator_pickle) ---------------------------------------------------------------------- Traceback (most recent call last): File "/Users/alexw/dev/cpython/Lib/test/test_deque.py", line 640, in test_iterator_pickle dump = pickle.dumps((itorg, orig), proto) _pickle.PicklingError: Can't pickle <class 'collections._deque_iterator'>: attribute lookup _deque_iterator on collections failed ----------------------------------------------------------------------

Sure, I added a comment.

AlexWaygood · 2024-06-12T19:15:51Z

+from _pyrepl.pager import (get_pager, pipe_pager,
+ plain_pager, tempfile_pager, tty_pager,
+ plain) # noqa


Which is the import being flagged as unused here? Why does it need to stay?

pydoc.plain() is an alias to _pyrepl.pager.plain(). It has to stay since it's part of pydoc API, and test_pydoc fails if you remove it.
# noqa only applies to the current line, no?

pydoc.plain() is an alias to _pyrepl.pager.plain(). It has to stay since it's part of pydoc API, and test_pydoc fails if you remove it.
I see, thanks. Maybe add a comment explaining that to the code?
# noqa only applies to the current line, no?
It depends on the tool, the rule and the AST node. For example, in this:
fromcollectionsimport ( # noqa: F401deque, OrderedDict, ChainMap, )
the single noqa comment will cause Ruff to ignore the three unused imports deque, OrderedDict and ChainMap, because they constitute a single ImportFrom node in the AST and the noqa comment is applied on the starting line number of the AST node.

I moved the import to a separated statement and added a comment.

gpshead · 2024-06-12T21:23:16Z

Instead of blanket everything # noqa comments, please make these specific to the thing being disabled. # noqa: unused-import for example. Except that ruff appears to not support human meaningful and thus maintainable suppression comments in favor of inhumane error code numbers in noqa comments? So that might read F401 today.

But at least we could fix those when ruff enters the modern era and names things to be understandable by code maintainers.

vstinner · 2024-06-12T21:47:02Z

Instead of blanket everything # noqa comments, please make these specific to the thing being disabled.

On a line like import readline # noqa, it's clear that noqa suppress a warning about unused import, no?

So that might read F401 today.

So you suggest using # noqa: F401 instead?

AlexWaygood · 2024-06-12T21:50:23Z

Except that ruff appears to not support human meaningful and thus maintainable suppression comments in favor of inhumane error code numbers in noqa comments? So that might read F401 today.
But at least we could fix those when ruff enters the modern era and names things to be understandable by code maintainers.

it's on our to-do list :-)

Edit: and I see Zanie already said that on the Ruff issue tracker ;)

AlexWaygood

I would also prefer that we ignore a specific linter error (F401) rather than all linter errors. But other than that, this LGTM now, thanks (though some tests seem to be failing)

gpshead · 2024-06-12T22:02:57Z

yep, always list the specific error(s) being ignored in the comment.

serhiy-storchaka

LGTM.

How did you know that all these imports were necessary? Or did you just flag all the names for which you can't confirm redundant imports?

vstinner · 2024-06-13T09:20:17Z

How did you know that all these imports were necessary? Or did you just flag all the names for which you can't confirm redundant imports?

I'm checking for unused imports for 5 years, and every year, I meet the same imports :-) Over time, I learnt which imports are done to populate an API, and which ones can be removed.

You can try to remove them and see the test suite failing.

serhiy-storchaka · 2024-06-13T09:26:05Z

I agree with all this, I just wondering how did you find them, because the amount of the work is large, and many cases are not trivial. Removing imports unlit the code breaks is risky, not all can be covered by tests.

vstinner · 2024-06-13T09:28:23Z

I modified the PR to use # noqa: F401. Hopefully, it seems like warning numbers are standardized between linters.

vstinner · 2024-06-13T09:30:09Z

I agree with all this, I just wondering how did you find them, because the amount of the work is large, and many cases are not trivial. Removing imports unlit the code breaks is risky, not all can be covered by tests.

I also rely on code review to validate my work :-)

hugovk · 2024-06-13T13:29:48Z

yep, always list the specific error(s) being ignored in the comment.

There's a rule (PGH004 blanket-noqa) to prevent noqas that are missing the specific error. We have it in https://github.com/python/cpython/blob/main/Tools/clinic/.ruff.toml:

cpython/Tools/clinic/.ruff.toml

Line 9 in ca5108a

"PGH004", # Ban blanket `# noqa` comments (only ignore specific error codes)

Shall we add it to the other one, https://github.com/python/cpython/blob/main/Lib/test/.ruff.toml ?

And maybe add another .ruff.toml for Lib/ or other directories?

I'm checking for unused imports for 5 years, and every year, I meet the same imports :-) Over time, I learnt which imports are done to populate an API, and which ones can be removed.

If we added F401 unused-import to Ruff, these would get flagged by the CI, and we'd have to label the ones we really need with noqa: F401.

vstinner · 2024-06-13T14:24:28Z

I created a second PR for the 7 files which requires tons of #noqa: #120454.

vstinner · 2024-06-13T14:25:33Z

Merged, thanks for reviews!

@hugovk: I'm interested by a CI checking for unused imports, it's appealing :-)

encukou · 2024-06-17T13:27:07Z

For things that are re-exported for a public API, should add them to __all__ rather than add the linter directive?

AlexWaygood · 2024-06-17T13:28:27Z

For things that are re-exported for a public API, should add them to __all__ rather than add the linter directive?

usually IMO, yes

Tools such as ruff can ignore "imported but unused" warnings if a line ends with "# noqa: F401". It avoids the temptation to remove an import which is used effectively.

pythongh-120417: Add #noqa to used imports in the stdlib
8347c30
Tools such as ruff can ignore "imported but unused" warnings if a line ends with "# noqa". It avoids the temptation to remove an import which is used effectively.

vstinner added the skip news label Jun 12, 2024

vstinner requested review from abalkin, gpshead, pganssle, rhettinger and tiran as code owners June 12, 2024 16:26

bedevere-appBot mentioned this pull request Jun 12, 2024
Remove unused imports from the stdlib #120417
Closed

bedevere-appBot added the awaiting core review label Jun 12, 2024

Add more # noqa for more fun
19ceb4d

vstinner requested review from berkerpeksag, brettcannon, ericsnowcurrently, erlend-aasland, ncoghlan and warsaw as code owners June 12, 2024 16:41

vstinner added 2 commits June 12, 2024 18:50

One more #noqa batch
9b97f47

Fix test_symtable
2189665

AlexWaygood reviewed Jun 12, 2024
View reviewed changes

Comment threadLib/importlib/machinery.py Outdated

vstinner added 2 commits June 12, 2024 21:02

Revert
e31a9e2

Merge branch 'main' into import_noqa
7d93caa

AlexWaygood reviewed Jun 12, 2024
View reviewed changes

AlexWaygood removed request for brettcannon, ericsnowcurrently, ncoghlan and warsaw June 12, 2024 19:30

Add comments
13efcc9

gpshead mentioned this pull request Jun 12, 2024
FR: Add human-friendly check/message ids. Numeric ids (E999 etc) are not human friendly - they communicate nothing on their own. astral-sh/ruff#11849
Closed

AlexWaygood approved these changes Jun 12, 2024
View reviewed changes

bedevere-appBot added awaiting merge and removed awaiting core review labels Jun 12, 2024

rhettinger removed their request for review June 12, 2024 22:20

pganssle reviewed Jun 13, 2024
View reviewed changes

Comment threadLib/operator.py Outdated

serhiy-storchaka approved these changes Jun 13, 2024
View reviewed changes

serhiy-storchaka reviewed Jun 13, 2024
View reviewed changes

Comment threadLib/pydoc.py Outdated

Update Lib/pydoc.py
648969c

Use "# noqa: F401"
0506006

vstinner merged commit 6ae254a into python:mainJun 13, 2024

vstinner deleted the import_noqa branch June 13, 2024 14:14

bedevere-appBot removed the awaiting merge label Jun 13, 2024

vstinner mentioned this pull request Jun 17, 2024
gh-120417: Add #noqa to load_tests() in tests #120626
Closed

vstinner mentioned this pull request Mar 13, 2025
Remove unused imports - March 2025 Edition #131152
Closed

Uh oh!

Uh oh!

Conversation

vstinner commented Jun 12, 2024• edited by bedevere-app BotLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlexWaygoodJun 12, 2024

Choose a reason for hiding this comment

Uh oh!

vstinnerJun 12, 2024

Choose a reason for hiding this comment

Uh oh!

AlexWaygoodJun 12, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinnerJun 12, 2024

Choose a reason for hiding this comment

Uh oh!

AlexWaygoodJun 12, 2024

Choose a reason for hiding this comment

Uh oh!

vstinnerJun 12, 2024

Choose a reason for hiding this comment

Uh oh!

AlexWaygoodJun 12, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinnerJun 12, 2024

Choose a reason for hiding this comment

Uh oh!

gpshead commented Jun 12, 2024

Uh oh!

vstinner commented Jun 12, 2024

Uh oh!

AlexWaygood commented Jun 12, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexWaygood left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gpshead commented Jun 12, 2024

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vstinner commented Jun 13, 2024

Uh oh!

serhiy-storchaka commented Jun 13, 2024

Uh oh!

vstinner commented Jun 13, 2024

Uh oh!

vstinner commented Jun 13, 2024

Uh oh!

hugovk commented Jun 13, 2024

Uh oh!

vstinner commented Jun 13, 2024

Uh oh!

vstinner commented Jun 13, 2024

Uh oh!

encukou commented Jun 17, 2024

Uh oh!

AlexWaygood commented Jun 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

vstinner commented Jun 12, 2024•
edited by bedevere-app Bot
Loading

AlexWaygoodJun 12, 2024•
edited
Loading

AlexWaygoodJun 12, 2024•
edited
Loading

AlexWaygood commented Jun 12, 2024•
edited
Loading

AlexWaygood left a comment •
edited
Loading