Skip to content

Conversation

@hauntsaninja
Copy link
Collaborator

Basically a follow up to #12203

New errors in typeshed from this:

_decimal.__libmpdec_version__ is not present in stub _heapq.__about__ is not present in stub builtins.__build_class__ is not present in stub cgitb.__UNDEF__ is not present in stub decimal.__libmpdec_version__ is not present in stub sys.__unraisablehook__ is not present in stub 

Some general housekeeping, moving things around, renaming things, adding
some comments.

New errors in typeshed from this: ``` _decimal.__libmpdec_version__ is not present in stub _heapq.__about__ is not present in stub builtins.__build_class__ is not present in stub cgitb.__UNDEF__ is not present in stub decimal.__libmpdec_version__ is not present in stub sys.__unraisablehook__ is not present in stub ``` Some general housekeeping, moving things around, renaming things, adding some comments.
@hauntsaninja
Copy link
CollaboratorAuthor

cc @AlexWaygood

"__new_member__", # If an enum defines __new__, the method is renamed as __new_member__
"__dataclass_fields__", # Generated by dataclasses
"__dataclass_params__", # Generated by dataclasses
"__doc__", # mypy's semanal for namedtuples assumes this is str, not Optional[str]
Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

This one should be a pretty easy fix

# TODO: remove the following from this list
"__author__",
"__version__",
"__copyright__",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"__copyright__",
"__copyright__",
"__about__",

Another one I noticed in my experiments recently!

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see you consider heap.__about__ a true positive?

Copy link
Member

Choose a reason for hiding this comment

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

I guess we could just put __about__: str in the stub. Doesn't do any harm.

Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

It's quite educational :D

Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

My reasoning here is that it's only clearly a true negative if the Python runtime is what's creating the attribute. Everything else you might conceivably want to have stubbed. I'm especially eager to remove __version__ from this list, which is widely used in third party stubs.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, that makes sense!

m
form, oinstub.names.items()
ifnoto.module_hiddenand (notm.startswith("_") orhasattr(runtime, m))
ifnoto.module_hiddenand (notis_probably_private(m) orhasattr(runtime, m))
Copy link
Member

Choose a reason for hiding this comment

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

I was planning something very similar, which was why I factored it out into a helper function even though it was only used once in my patch 😆

@github-actions

This comment has been minimized.

@hauntsaninja
Copy link
CollaboratorAuthor

This causes us to error if __all__ is in stub, but not in runtime.
It's possible that's undesirable? But we'll be thinking about __all__ very soon (sorry for merge conflicts!)

@AlexWaygood
Copy link
Member

(sorry for merge conflicts!)

😡

@AlexWaygood
Copy link
Member

It's possible that's undesirable?

I think it should be fine? I can't think of any scenarios off the top of my head where that would be a problem 🙂

@hauntsaninja
Copy link
CollaboratorAuthor

hauntsaninja commented Feb 20, 2022

I was thinking something like: you're adding stubs for a third party module and want to use __all__ as a way to tell type checkers what the public interface is meant to be. (The only alternative being not adding stubs for private stuff at all)

@AlexWaygood
Copy link
Member

Why don't we just add __all__ to IGNORED_MODULE_DUNDERS for now? It's no change from the status quo, and, as you say, we'll be thinking properly about __all__ very soon.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@hauntsaninja
Copy link
CollaboratorAuthor

That would be a regression from the status quo, since we wouldn't check __all__ even if the stub had it. So the bare minimum tuple length errors we had go away. I'll merge this, this might be the kind of problem where we wait to see if anyone ever reports it.

@hauntsaninjahauntsaninja merged commit 58514a9 into python:masterFeb 20, 2022
@hauntsaninjahauntsaninja deleted the stubtdund branch February 20, 2022 02:08
Sign up for freeto 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.

2 participants

@hauntsaninja@AlexWaygood