Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-120417: Add #noqa to used imports in the stdlib#120421

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
vstinner merged 9 commits intopython:mainfromvstinner:import_noqa
Jun 13, 2024

Conversation

@vstinner
Copy link
Member

@vstinnervstinner commentedJun 12, 2024
edited by bedevere-appbot
Loading

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.

Tools such as ruff can ignore "imported but unused" warnings if aline ends with "# noqa". It avoids the temptation to remove an importwhich is used effectively.
Copy link
Member

@AlexWaygoodAlexWaygood left a comment

Choose a reason for hiding this comment

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

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

Comment on lines 48 to 51
try:
from_collectionsimport_deque_iterator
from_collectionsimport_deque_iterator# noqa
exceptImportError:
pass
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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

Copy link
Member

@AlexWaygoodAlexWaygoodJun 12, 2024
edited
Loading

Choose a reason for hiding this comment

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

Oh,test_deque fails if the import is removed. I expecteddeque to be tested as part oftest_collections, but apparently it has its own test file. Could you add a comment that it is (apparently) required to expose it in thecollections 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", line640, intest_iterator_pickle    dump= pickle.dumps((itorg, orig), proto)_pickle.PicklingError:Can't pickle <class 'collections._deque_iterator'>: attribute lookup _deque_iterator on collections failed----------------------------------------------------------------------

erlend-aasland reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Sure, I added a comment.

AlexWaygood reacted with heart emoji
Lib/pydoc.py Outdated
Comment on lines 78 to 80
from_pyrepl.pagerimport (get_pager,pipe_pager,
plain_pager,tempfile_pager,tty_pager,
plain)# noqa
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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?

Copy link
Member

@AlexWaygoodAlexWaygoodJun 12, 2024
edited
Loading

Choose a reason for hiding this comment

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

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 singlenoqa comment will cause Ruff to ignore the three unused importsdeque,OrderedDict andChainMap, because they constitute a singleImportFrom node in the AST and thenoqa comment is applied on the starting line number of the AST node.

gpshead reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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

AlexWaygood reacted with heart emoji
@gpshead
Copy link
Member

Instead of blanket everything# noqa comments, please make these specific to the thing being disabled.# noqa: unused-import for example. Except thatruff 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.

AlexWaygood, aisk, and erlend-aasland reacted with thumbs up emoji

@vstinner
Copy link
MemberAuthor

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

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

So that might read F401 today.

So you suggest using# noqa: F401 instead?

@AlexWaygood
Copy link
Member

AlexWaygood commentedJun 12, 2024
edited
Loading

Except thatruff 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 ;)

gpshead reacted with heart emoji

Copy link
Member

@AlexWaygoodAlexWaygood left a comment
edited
Loading

Choose a reason for hiding this comment

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

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

gpshead reacted with thumbs up emoji
@gpshead
Copy link
Member

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

@rhettingerrhettinger removed their request for reviewJune 12, 2024 22:20
Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

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

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
Copy link
MemberAuthor

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.

AlexWaygood reacted with thumbs up emoji

@serhiy-storchaka
Copy link
Member

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
Copy link
MemberAuthor

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

@vstinner
Copy link
MemberAuthor

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 :-)

AlexWaygood reacted with heart emoji

@hugovk
Copy link
Member

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

There's a rule (PGH004blanket-noqa) to preventnoqas that are missing the specific error. We have it inhttps://github.com/python/cpython/blob/main/Tools/clinic/.ruff.toml:

"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 forLib/ 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 addedF401unused-import to Ruff, these would get flagged by the CI, and we'd have to label the ones we really need withnoqa: F401.

erlend-aasland reacted with thumbs up emoji

@vstinnervstinner merged commit6ae254a intopython:mainJun 13, 2024
@vstinnervstinner deleted the import_noqa branchJune 13, 2024 14:14
@vstinner
Copy link
MemberAuthor

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

@vstinner
Copy link
MemberAuthor

Merged, thanks for reviews!

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

erlend-aasland reacted with thumbs up emoji

@encukou
Copy link
Member

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

@AlexWaygood
Copy link
Member

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

usually IMO, yes

mrahtz pushed a commit to mrahtz/cpython that referenced this pull requestJun 30, 2024
Tools such as ruff can ignore "imported but unused" warnings if aline ends with "# noqa: F401". It avoids the temptation to removean import which is used effectively.
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull requestJul 11, 2024
Tools such as ruff can ignore "imported but unused" warnings if aline ends with "# noqa: F401". It avoids the temptation to removean import which is used effectively.
estyxx pushed a commit to estyxx/cpython that referenced this pull requestJul 17, 2024
Tools such as ruff can ignore "imported but unused" warnings if aline ends with "# noqa: F401". It avoids the temptation to removean import which is used effectively.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@pgansslepgansslepganssle left review comments

@serhiy-storchakaserhiy-storchakaserhiy-storchaka approved these changes

@AlexWaygoodAlexWaygoodAlexWaygood approved these changes

@gpsheadgpsheadAwaiting requested review from gpsheadgpshead is a code owner

@abalkinabalkinAwaiting requested review from abalkinabalkin is a code owner

@tirantiranAwaiting requested review from tiran

@berkerpeksagberkerpeksagAwaiting requested review from berkerpeksagberkerpeksag is a code owner

@erlend-aaslanderlend-aaslandAwaiting requested review from erlend-aaslanderlend-aasland is a code owner

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@vstinner@gpshead@AlexWaygood@serhiy-storchaka@hugovk@encukou@pganssle

[8]ページ先頭

©2009-2025 Movatter.jp