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

Globally imported dunders are implicitly used. #645

Closed
wants to merge 1 commit into from

Conversation

anntzer
Copy link

@anntzer anntzer commented Jul 4, 2021

... to support e.g. importing __version__ from an autogenerated
submodule.

Closes #387.

I chose to skip all dunders (per https://www.python.org/dev/peps/pep-0008/#module-level-dunder-names), but I can also just special-case __version__, whatever you prefer.

... to support e.g. importing `__version__` from an autogenerated
submodule.
@asottile
Copy link
Member

asottile commented Jul 4, 2021

I'm -1 -- add these to __all__ like any other variable and they'll be marked as used

@anntzer
Copy link
Author

anntzer commented Jul 4, 2021

As discussed in #387 (comment) adding them to __all__ is not a good idea (you almost certainly don't want them imported if doing a star-import).

@asottile
Copy link
Member

asottile commented Jul 4, 2021

import * is a bad idea anyway

@anntzer
Copy link
Author

anntzer commented Jul 4, 2021

I agree, but that's what __all__ is fundamentally about...

@asottile
Copy link
Member

asottile commented Jul 4, 2021

not entirely, it's also for documenting the "public" names of a namespace (tab completion, doc generation, etc.)

@anntzer
Copy link
Author

anntzer commented Jul 4, 2021

Fair, but note that https://www.python.org/dev/peps/pep-0008/#module-level-dunder-names specifically does not consider __version__ to be worth adding to __all__.

@asottile
Copy link
Member

asottile commented Jul 4, 2021

yeah it also imports barry_as_FLUFL -- I don't think it's a serious example

@anntzer
Copy link
Author

anntzer commented Jul 4, 2021

It's not a very serious example, but it's also the only thing (AFAICT) in the reference docs that addresses this point...

@sigmavirus24
Copy link
Member

I'm also a fairly strong -1 on this. import * has been discouraged for the greater part of a decade if not more.

@anntzer
Copy link
Author

anntzer commented Jul 4, 2021

To be clear, this is not really about import *, but about how to reuse __version__ defined in a helper module.

@sigmavirus24
Copy link
Member

Yes, I understand that. Your argument against what seems to be a common practice is potential use of an uncommon one that's strongly recommended against

@anntzer
Copy link
Author

anntzer commented Jul 4, 2021

Well, another argument given above is that PEP8 explicitly does not add "__version__" to __all__. Yes, the example is clearly a bit tongue-in-cheek, but, given its clean history (it was added in a single chunk in python/peps@070a23b), it seems clear that its author did not consider "__version__" as being part of __all__ and that it's not just an omission.


edit: Also, it is not clear to me why you think the behavior proposed here would be harmful; i.e., do you think it can ever give rise to incorrectly missed warnings? To be clear, I am more than willing to restrict the special-casing to __version__ rather than all dunders (I don't really care either way); assuming that I do so, the question becomes exactly "Does pyflakes want to warn when people write from [.]foo import __version__ / from [.]foo import version as __version__ or does pyflakes want to force them to split this over two lines (import foo; __version__ = foo.version or similar)".

@sigmavirus24
Copy link
Member

but, given its clean history

This is not a convincing argument. The PEP was a point-in-time capture of how to write code for the standard library which pycodestyle attempts to enforce (despite the sometimes split-brain nature of the document), not pyflakes. This project looks for high confidence lints that it can report.

I am more than willing to restrict the special-casing to __version__ rather than all dunders (I don't really care either way)

There are many projects that use __about__ or __metadata__ as a module that they import into __init__ to then re-export things. Of those projects I believe many have far more "dunder" variables than just __version__. Special-casing __version__ opens the door for PRs to special-case more things. Ignoring everything, opens the doors to bug reports because they stopped using a dunder variable and we didn't report it as unused.

There seems to be little possibility for anything other than an absolute increase in maintainer-burden by accepting some variant of this pull request.

@anntzer
Copy link
Author

anntzer commented Jul 5, 2021

This project looks for high confidence lints that it can report.

My second point in my previous message is that this is with high confidence an invalid lint, and that it artificially forces a perfectly valid single-statement construct to be split into two statements for the sole purpose of satisfying pyflakes. (On top of that, the two statements will typically have to be far away from one another as one will be in the imports block whereas the other defines a global variable.)

an absolute increase in maintainer-burden

I would argue that the increase is not large (and is counterbalanced by a real usability improvement), although I am obviously not a maintainer, so my judgment hardly matters here.

@asottile asottile closed this Nov 24, 2022
@anntzer anntzer deleted the implicitly-used-dunders branch November 24, 2022 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Treat global dunder names as always used
3 participants