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

Allow instances of a class to access inner class definitions on that class. #3636

Merged
merged 2 commits into from Jun 30, 2017

Conversation

Projects
None yet
4 participants
@rowillia
Contributor

rowillia commented Jun 29, 2017

This fixes #3635

@ilevkivskyi

The solution is a bit hacky, since it seems to me such things should be done in semanal.py, but maybe there will be other problems, so that I am fine with this.

@ethanhs

This comment has been minimized.

Show comment
Hide comment
@ethanhs

ethanhs Jun 29, 2017

Collaborator

I too had similar concerns on further consideration, however, I cannot think of another case where an inner class might cause issues like this. Though I suppose not considering uses of inner classes is exactly what caused the bug. I think it would be better to fix semanal, as it would avoid potential future issues. Also I don't like the idea of mypy "lying" about the type, as it makes the code harder to understand unless you are familiar with this hack.

Collaborator

ethanhs commented Jun 29, 2017

I too had similar concerns on further consideration, however, I cannot think of another case where an inner class might cause issues like this. Though I suppose not considering uses of inner classes is exactly what caused the bug. I think it would be better to fix semanal, as it would avoid potential future issues. Also I don't like the idea of mypy "lying" about the type, as it makes the code harder to understand unless you are familiar with this hack.

Show outdated Hide outdated test-data/unit/check-classes.test
reveal_type(Foo.Meta.name) # E: Revealed type is 'builtins.str'
reveal_type(Foo().meta.name) # E: Revealed type is 'builtins.str'
reveal_type(Foo().Meta.name) # E: Revealed type is 'builtins.str'

This comment has been minimized.

@gvanrossum

gvanrossum Jun 30, 2017

Member

Why not test Foo.meta.name too?

And what will the types of Foo.Meta, Foo().Meta, Foo.meta and Foo().meta be revealed to be?

@gvanrossum

gvanrossum Jun 30, 2017

Member

Why not test Foo.meta.name too?

And what will the types of Foo.Meta, Foo().Meta, Foo.meta and Foo().meta be revealed to be?

This comment has been minimized.

@rowillia

rowillia Jun 30, 2017

Contributor

Thanks for the feedback! Added those tests in.

@rowillia

rowillia Jun 30, 2017

Contributor

Thanks for the feedback! Added those tests in.

@gvanrossum gvanrossum merged commit 2f3d4a2 into python:master Jun 30, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment