Skip to content

Make get_type_hints not throw an error on builtin methods#368

Merged
ilevkivskyi merged 3 commits into
python:masterfrom
wheerd:master
Feb 10, 2017
Merged

Make get_type_hints not throw an error on builtin methods#368
ilevkivskyi merged 3 commits into
python:masterfrom
wheerd:master

Conversation

@wheerd

@wheerd wheerd commented Jan 26, 2017

Copy link
Copy Markdown
Contributor

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

ilevkivskyi commented Jan 26, 2017

Copy link
Copy Markdown
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

ilevkivskyi commented Jan 27, 2017

Copy link
Copy Markdown
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?

methane
methane reviewed Jan 30, 2017
View reviewed changes
Comment thread src/typing.py Outdated
isinstance(obj, types.ModuleType)
isinstance(obj, types.ModuleType) or
isinstance(obj, _wrapper_descriptor_type) or
isinstance(obj, _method_wrapper_type)

@methane methane Jan 30, 2017

Copy link
Copy Markdown
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):

@ilevkivskyi ilevkivskyi Jan 30, 2017

Copy link
Copy Markdown
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

ilevkivskyi commented Feb 10, 2017

Copy link
Copy Markdown
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

wheerd commented Feb 10, 2017

Copy link
Copy Markdown
Contributor Author

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

@ilevkivskyi

ilevkivskyi commented Feb 10, 2017

Copy link
Copy Markdown
Member

Thanks!

@ilevkivskyi ilevkivskyi merged commit 440dcde into python:master Feb 10, 2017
@JGiard JGiard mentioned this pull request Feb 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ilevkivskyi ilevkivskyi ilevkivskyi left review comments

+1 more reviewer

@methane methane methane left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CLA signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@wheerd @ilevkivskyi @methane @the-knights-who-say-ni