Skip to content

Mark object.__module__ as ClassVar #8787

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

Closed
wants to merge 1 commit into from

Conversation

hauntsaninja
Copy link
Collaborator

@hauntsaninja hauntsaninja commented Sep 23, 2022

The reported use case for this isn't particularly compelling, so if this causes too many errors I will close.

@AlexWaygood
Copy link
Member

I kinda think object.__module__ shouldn't actually exist:

>>> object().__module__
Traceback (most recent call last):
  File "<string>", line 1, in <module>
AttributeError: 'object' object has no attribute '__module__'

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

streamlit (https://github.com/streamlit/streamlit)
+ lib/tests/streamlit/runtime/caching/hashing_test.py:29: note: ... from here:

SinbadCogs (https://github.com/mikeshardmind/SinbadCogs)
+ devtools/core.py:193: error: Cannot assign to class variable "__module__" via instance

yarl (https://github.com/aio-libs/yarl)
+ yarl/_url.py:19: error: Cannot assign to class variable "__module__" via instance  [misc]

werkzeug (https://github.com/pallets/werkzeug)
+ src/werkzeug/utils.py:89: error: Cannot assign to class variable "__module__" via instance  [misc]

@hauntsaninja
Copy link
Collaborator Author

hauntsaninja commented Sep 23, 2022

Looking at them, I'm fine with taking these primer hits, but I'm a little worried about all the stubs that have this.

I'm happy to have object.__module__ exist, it's in line with several things we do. There's a small chance removing it wouldn't be the worst, though, since we still have __module__ on type.

@AlexWaygood
Copy link
Member

I'm happy to have object.__module__ exist, it's in line with several things we do.

I know, but I'm not sure I'm super jazzed about those things either ¯\_(ツ)_/¯

AlexWaygood added a commit that referenced this pull request Sep 24, 2022
An alternative approach to #8787
@AlexWaygood
Copy link
Member

There's a small chance removing it wouldn't be the worst, though, since we still have __module__ on type.

#8789 went down like a bag of sick, so if we want to fix this, this does indeed look like the better solution, at least for now :)

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.

2 participants