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

Support module-level __getattr__ #3647

Merged
merged 4 commits into from Jul 18, 2017

Conversation

Projects
None yet
4 participants
@JelleZijlstra
Collaborator

JelleZijlstra commented Jul 2, 2017

Fixes #2496

PEP 484 specifies that stubs can add a global __getattr__ function to indicate that they have arbitrary other attributes. See python/typeshed#5 and python/typing#181.

This implementation goes beyond the spec in one way:

  • If the module-level __getattr__ has a more precise return type than Any, we use it.
@JelleZijlstra

This comment has been minimized.

Show comment
Hide comment
@JelleZijlstra

JelleZijlstra Jul 2, 2017

Collaborator

I think the Travis failure is a flake.

Collaborator

JelleZijlstra commented Jul 2, 2017

I think the Travis failure is a flake.

@ethanhs

This comment has been minimized.

Show comment
Hide comment
@ethanhs

ethanhs Jul 2, 2017

Collaborator

The Travis flakiness is rather annoying. I think I will get in touch with them and see if they have any ideas of why it is doing that. As for the PR, I left a couple of comments, I think the only big question is the scope of support for this, how limited/free we want to make that. I think the PEP might need some clarification either way.

Collaborator

ethanhs commented Jul 2, 2017

The Travis flakiness is rather annoying. I think I will get in touch with them and see if they have any ideas of why it is doing that. As for the PR, I left a couple of comments, I think the only big question is the scope of support for this, how limited/free we want to make that. I think the PEP might need some clarification either way.

Show outdated Hide outdated test-data/unit/check-modules.test
[case testModuleLevelGetattribute]
def __getattribute__(): ... # E: __getattribute__ is not valid at the module level

This comment has been minimized.

@ethanhs

ethanhs Jul 2, 2017

Collaborator

Another potential test: an untyped __getattr__, and if the implementation is changed to refuse it in Python files, a test for that of course.

@ethanhs

ethanhs Jul 2, 2017

Collaborator

Another potential test: an untyped __getattr__, and if the implementation is changed to refuse it in Python files, a test for that of course.

This comment has been minimized.

@JelleZijlstra

JelleZijlstra Jul 2, 2017

Collaborator

Adding the first one.

@JelleZijlstra

JelleZijlstra Jul 2, 2017

Collaborator

Adding the first one.

fix Ethan's comments
- Ignore __getattr__ if it's not a function.
- Test for untyped __getattr__ (and treat it as returning Any).

Thanks for the comments!
@ethanhs

ethanhs approved these changes Jul 6, 2017

@ethanhs

This comment has been minimized.

Show comment
Hide comment
@ethanhs

ethanhs Jul 6, 2017

Collaborator

I approved changes, but I think it'd be good to hear from @JukkaL or @gvanrossum on clarifying the PEP.
(See discussion here). The question is that the PEP states:

  • Stub files may be incomplete. To make type checkers aware of this, the file can contain the following code:
    def __getattr__(name) -> Any: ...
    Any identifier not defined in the stub is therefore assumed to be of type Any .

The implementation here adds support for it in not just stubs but also Python files. I think this is okay as long as the PEP clarifies this, or we think it is okay for MyPy to go beyond the PEP. Thoughts?

Collaborator

ethanhs commented Jul 6, 2017

I approved changes, but I think it'd be good to hear from @JukkaL or @gvanrossum on clarifying the PEP.
(See discussion here). The question is that the PEP states:

  • Stub files may be incomplete. To make type checkers aware of this, the file can contain the following code:
    def __getattr__(name) -> Any: ...
    Any identifier not defined in the stub is therefore assumed to be of type Any .

The implementation here adds support for it in not just stubs but also Python files. I think this is okay as long as the PEP clarifies this, or we think it is okay for MyPy to go beyond the PEP. Thoughts?

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Jul 6, 2017

Member

In stubs you can can lie about various things. But I don't think we should allow __getattr__ outside stubs, because this most definitely doesn't work (it will not be called when you ask for an attribute on the module object).

Member

gvanrossum commented Jul 6, 2017

In stubs you can can lie about various things. But I don't think we should allow __getattr__ outside stubs, because this most definitely doesn't work (it will not be called when you ask for an attribute on the module object).

@JelleZijlstra

This comment has been minimized.

Show comment
Hide comment
@JelleZijlstra

JelleZijlstra Jul 15, 2017

Collaborator

Just pushed a change to only allow module-level __getattr__ in stubs.

Collaborator

JelleZijlstra commented Jul 15, 2017

Just pushed a change to only allow module-level __getattr__ in stubs.

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Jul 18, 2017

Collaborator

It looks like this now conforms to what Guido said, so I am going to merge this.

Collaborator

ilevkivskyi commented Jul 18, 2017

It looks like this now conforms to what Guido said, so I am going to merge this.

@ilevkivskyi ilevkivskyi merged commit 3814b1a into python:master Jul 18, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@JelleZijlstra JelleZijlstra deleted the JelleZijlstra:modulegetattr branch Jul 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment