Skip to content

Make get_type_hints not throw an error on builtin methods #368

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

Merged
merged 3 commits into from
Feb 10, 2017

Conversation

wheerd
Copy link
Contributor

@wheerd wheerd commented Jan 26, 2017

While builtin functions are supported, calling get_type_hints on a builtin method like object.__init__ causes an error. I change this to return an empty dictionary instead.

@ilevkivskyi
Copy link
Member

Thank you for the PR!
I see that you also have opened an issue on b.p.o. http://bugs.python.org/issue29377 to add the types you need here to types module. I would prefer that these types are first added to types, and then you update this PR to use try: from types import ... except ImportError: WrapperDescriptorType = ...

@ilevkivskyi
Copy link
Member

@wheerd After you take a look at my latest review on b.p.o., could you please update this PR using the try: from types import ... idiom I mentioned above and fix the lint issue?

src/typing.py Outdated
isinstance(obj, types.ModuleType)
isinstance(obj, types.ModuleType) or
isinstance(obj, _wrapper_descriptor_type) or
isinstance(obj, _method_wrapper_type)
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this isinstance chain.
How about this?

# global
_annotatable_types = (
    types.FunctionType,
    types.BuiltinFunctionType,
    types.MethodType,
    types.ModuleType,
    _wrapper_descriptor_type,
    _method_wrapper_type)

...

        if isinstance(obj, _annotatable_types):

Copy link
Member

Choose a reason for hiding this comment

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

Thank, you! I have already noticed this some time ago, I would prefer to fix this separately after this PR is merged.

@ilevkivskyi
Copy link
Member

@wheerd Could you please update the PR using the try: from types import ... idiom I mentioned in my first comment and fix the lint issue? Otherwise we could not move forward with this PR (I can't do this myself, since I can't merge a failing PR).

@wheerd
Copy link
Contributor Author

wheerd commented Feb 10, 2017

Sorry I completely forgot about the pull request. Should be fixed now.

@ilevkivskyi
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

4 participants