Skip to content
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

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

Closed
wants to merge 1 commit into from

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jun 13, 2024

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.

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.
@vstinner
Copy link
Member Author

@AlexWaygood
Copy link
Member

I'm still against this approach, except for maybe Lib/collections/abc.py, where there are only two being noqa'd. If we need to ignore this many imports in a file, there are lots of other workarounds:

  • Add __all__ to the file so it's clear to the linter what's being deliberately re-exported
  • Ignore the whole file by putting # noqa: F401 on a line by itself near the top of the file
  • Use a --exclude CLI flag when running the linter to exclude the whole file from the check.

In my opinion, this is just too ugly for a file like ctypes/__init__.py.

@ericsnowcurrently
Copy link
Member

tl;dr I strongly favor using __all__ for nearly all of these cases.

In a very few of the cases I'd say the linters aren't able to do a good job of identifying false positives for F401. However, IMHO the linter is actually doing the right thing in the remaining cases. In that vast majority it is simply exposing the fact that we aren't using __all__ when we should be. If we used __all__ the way we should then there would be no need for #noqa, etc. in those cases.

Here we're dealing with a whole lot of bound objects that are meant to be available to users as part of a module's API. __all__ is exactly the place where you indicate which bound objects are available to users. 1 All objects that are imported just to be exposed as-is (I usually call them "aliases") fall into this category. __all__ is the natural place to identify them, to effectively say "these objects were imported to be exposed by this module."

So why are we avoiding __all__ for all these "alias" cases?

Footnotes

  1. I'd argue that having __all__ is a good idea generally; it makes the module's public API explicit to users/maintainers and to the runtime, available on the module for code to easily look at.

@serhiy-storchaka
Copy link
Member

This is pretty common idiom -- defining the names in the submodules and exposing them as a public API at the package level.

Ideally, linters should be aware of this idiom.

@vstinner
Copy link
Member Author

Here is a different approach to fix these warnings: #120461 PR adding __all__ to modules. I left ctypes and ssl aside for now.

@gpshead
Copy link
Member

gpshead commented Jun 13, 2024

Thanks Eric, agreed, I like the ultimate explicitness of __all__. (I was primarily intending to point out that we should introduce it with that specific intent in my earlier comment)

(as for improvements to static analysis like linters, lets just file those as FRs in their respective trackers - ruff is still young and lacks some practical things)

@rhettinger rhettinger removed their request for review June 14, 2024 17:29
@vstinner
Copy link
Member Author

I closed this PR: #120461 looks like a better approach.

@vstinner vstinner closed this Jun 14, 2024
@vstinner vstinner deleted the import_qa2 branch June 14, 2024 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants