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

Use the get_attribute hook for dynamic classes #6371

Merged
merged 9 commits into from Feb 10, 2019

Conversation

Projects
None yet
2 participants
@lincolnq
Copy link
Contributor

commented Feb 10, 2019

Whenever we find __getattr__ or __getattribute__ on an instance, the getattribute hook is called to give the hook a chance to modify the return type of one of those functions.

Closes #6259, #5910

(Note: Currently there are no tests. I can add them if important.)

lincolnq added some commits Feb 10, 2019

Use the get_attribute hook for dynamic classes
Whenever we find __getattr__ or __getattribute__ on an instance, the getattribute hook is used.
(It continues to be used whenever we find a method or variable on an instance)

Closes #6259, #5910
@ilevkivskyi
Copy link
Collaborator

left a comment

Thanks for PR! I have couple comments. Could you also please add tests? (You can look at the existing tests for plugin hooks to see how to add them.)

Show resolved Hide resolved mypy/checkmember.py Outdated
Show resolved Hide resolved docs/source/extending_mypy.rst Outdated

@lincolnq lincolnq referenced this pull request Feb 10, 2019

Open

[WIP] Implement "Maybe[T]" type #1

0 of 5 tasks complete
@lincolnq

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2019

@ilevkivskyi Thanks for the review! I made the changes you requested.

One of the test cases is that the attribute hook can produce an error. I invented my own error, "Field does not exist", but ideally I would test that you can fallback to the normal mypy error handling -- but I don't think you can access a MemberContext to call mx.msg.has_no_attr. Is the way I did it ok?

@ilevkivskyi
Copy link
Collaborator

left a comment

Thanks for updates!

Sorry, there is one more thing, could you please also update the docstring for get_attribute_hook() in mypy/plugin.py?

@lincolnq

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2019

lincolnq added some commits Feb 10, 2019

Add a (failing) test case for the base class name being used for the …
…dynamic class.

- Also start updating the docstring in plugin.py, although not sure yet what proper behavior is
Pass the class where __getattr__ is defined as the class for the hook…
… implementation, not the class of the type of the field we are resolving

- Also make the other test I wrote work correctly.
@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

commented Feb 10, 2019

I assume the documented behavior is the desirable one?

Yes, I think giving the name of the base class where __getattr__() was defined would be consistent. So you probably need to update your code. (The fix for the old test I posted above.)

@ilevkivskyi ilevkivskyi merged commit 2a32d73 into python:master Feb 10, 2019

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
You can’t perform that action at this time.