Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.7k
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
Uh oh!
There was an error while loading.Please reload this page.
AlexWaygood left a comment
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! Looks much better now, a couple of questions though:
Uh oh!
There was an error while loading.Please reload this page.
| try: | ||
| from_collectionsimport_deque_iterator | ||
| from_collectionsimport_deque_iterator# noqa | ||
| exceptImportError: | ||
| pass |
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.
test_collections passes for me if I remove this import. Does it need to stay? It was added in52f96d3
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.
@erlend-aasland@kumaraditya303: Do you know/recall why this symbol is exposed?
AlexWaygoodJun 12, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
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----------------------------------------------------------------------
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.
Sure, I added a comment.
Lib/pydoc.py Outdated
| from_pyrepl.pagerimport (get_pager,pipe_pager, | ||
| plain_pager,tempfile_pager,tty_pager, | ||
| plain)# noqa |
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.
Which is the import being flagged as unused here? Why does it need to stay?
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.
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?
AlexWaygoodJun 12, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
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?
# noqaonly 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.
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.
I moved the import to a separated statement and added a comment.
gpshead commentedJun 12, 2024
Instead of blanket everything But at least we could fix those when ruff enters the modern era and names things to be understandable by code maintainers. |
vstinner commentedJun 12, 2024
On a line like
So you suggest using |
AlexWaygood commentedJun 12, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
it's on our to-do list :-) Edit: and I see Zanie already said that on the Ruff issue tracker ;) |
AlexWaygood left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
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 commentedJun 12, 2024
yep, always list the specific error(s) being ignored in the comment. |
Uh oh!
There was an error while loading.Please reload this page.
serhiy-storchaka left a comment
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.
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?
Uh oh!
There was an error while loading.Please reload this page.
vstinner commentedJun 13, 2024
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 commentedJun 13, 2024
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 commentedJun 13, 2024
I modified the PR to use |
vstinner commentedJun 13, 2024
I also rely on code review to validate my work :-) |
hugovk commentedJun 13, 2024
There's a rule (PGH004 cpython/Tools/clinic/.ruff.toml Line 9 inca5108a
Shall we add it to the other one,https://github.com/python/cpython/blob/main/Lib/test/.ruff.toml ? And maybe add another
If we addedF401 |
vstinner commentedJun 13, 2024
I created a second PR for the 7 files which requires tons of |
vstinner commentedJun 13, 2024
Merged, thanks for reviews! @hugovk: I'm interested by a CI checking for unused imports, it's appealing :-) |
encukou commentedJun 17, 2024
For things that are re-exported for a public API, should add them to |
AlexWaygood commentedJun 17, 2024
usually IMO, yes |
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.
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.
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.
Uh oh!
There was an error while loading.Please reload this page.
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.