get_type_hints(): find the right globalns for classes and modules by ambv · Pull Request #470 · python/typing (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation11 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This makes the default behavior (without specifying globalns manually) more
predictable for users.

Implementation for classes assumes has a __module__ attribute and that module
is present in sys.modules. It does this recursively for all bases in the
MRO. For modules, the implementation just uses their __dict__ directly.

This is backwards compatible, will just raise fewer exceptions in naive user
code.

@@ -1494,7 +1495,10 @@ def get_type_hints(obj, globalns=None, localns=None):
if getattr(obj, '__no_type_check__', None):
return {}
if globalns is None:
globalns = getattr(obj, '__globals__', {})
if isinstance(obj, type):
globalns = vars(sys.modules[obj.__module__])

Choose a reason for hiding this comment

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

While you are at it maybe you can fix same way also the logic below when __mro__ is traversed
(I could imagine that base classes are from different modules). Or will it be too hard? If not, I would add also a test for such scenario.

Choose a reason for hiding this comment

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

Yeah, I have a few ideas more but it's annoying that we don't ship ann_module.py et al. but always refer to the one in test for Python 3.5+. This makes it hard to iterate on them.

Let me see what I can do.

Choose a reason for hiding this comment

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

Done!

ambv changed the title~~get_type_hints(): find the right globalns for classes~~ get_type_hints(): find the right globalns for classes and modules

Sep 13, 2017

# name coming from different modules in the same program.
mgc_hints = {'default_a': Optional[mod_generics_cache.A],
'default_b': Optional[mod_generics_cache.B]}
self.assertEqual(gth(mod_generics_cache), mgc_hints)

Choose a reason for hiding this comment

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

I will fix this bug in a separate PR when this one lands.

Choose a reason for hiding this comment

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

I will fix this bug in a separate PR when this one lands.

OK, just don't forget about it (or open an issue if you want to do this later).

Choose a reason for hiding this comment

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

Hi, I was looking at a different issue and noticed this test code behaving non-deterministically depending on when mod_generics_cache was loaded (specifically this test would expectedly fail when I just ran the test_typing module, but unexpectedly succeed when I ran ./python -m test.)

Fixing the underlying type hint caching is beyond my scope as a new contributor, but it seems like something like this would make this test behave more reliably, if that seems reasonable to y'all: natgaertner/cpython@0cde103

This makes the default behavior (without specifying globalns manually) more predictable for users.

Implementation for classes assumes has a __module__ attribute and that module is present in sys.modules. It does this recursively for all bases in the MRO. For modules, the implementation just uses their __dict__ directly.

This is backwards compatible, will just raise fewer exceptions in naive user code.

@@ -1,14 +1,53 @@
"""Module for testing the behavior of generics across different modules."""

Choose a reason for hiding this comment

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

I only added a bunch of variable annotations here but since this wasn't compatible with < 3.5, I had to uglify this module. Now it's actually easier to see what's going on just by looking at the entire file with the "View" button.

# name coming from different modules in the same program.
mgc_hints = {'default_a': Optional[mod_generics_cache.A],
'default_b': Optional[mod_generics_cache.B]}
self.assertEqual(gth(mod_generics_cache), mgc_hints)

Choose a reason for hiding this comment

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

I will fix this bug in a separate PR when this one lands.

OK, just don't forget about it (or open an issue if you want to do this later).

Also, will this go into 3.6.3?

Yes, that's the point. I'm working on the backport as you read this.

Hopefully I'll get to fix the invalid cache of forward refs, too, before 2017-09-18 (PEP 494 3.6.3 RC).

ambv pushed a commit to ambv/cpython that referenced this pull request

Sep 14, 2017

…ules

This makes the default behavior (without specifying globalns manually) more predictable for users, finds the right globalns automatically.

Implementation for classes assumes has a __module__ attribute and that module is present in sys.modules. It does this recursively for all bases in the MRO. For modules, the implementation just uses their __dict__ directly.

This is backwards compatible, will just raise fewer exceptions in naive user code.

Originally implemented and reviewed at python/typing#470.

ambv mentioned this pull request

Sep 14, 2017

Hopefully I'll get to fix the invalid cache of forward refs, too, before 2017-09-18 (PEP 494 3.6.3 RC).

Yes, it would be great if this gets in too.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Sep 14, 2017

…nd modules (pythonGH-3582)

This makes the default behavior (without specifying globalns manually) more predictable for users, finds the right globalns automatically.

Implementation for classes assumes has a __module__ attribute and that module is present in sys.modules. It does this recursively for all bases in the MRO. For modules, the implementation just uses their __dict__ directly.

This is backwards compatible, will just raise fewer exceptions in naive user code.

Originally implemented and reviewed at python/typing#470. (cherry picked from commit f350a26)

ambv added a commit to python/cpython that referenced this pull request

Sep 14, 2017

…ules (#3582)

This makes the default behavior (without specifying globalns manually) more predictable for users, finds the right globalns automatically.

Implementation for classes assumes has a __module__ attribute and that module is present in sys.modules. It does this recursively for all bases in the MRO. For modules, the implementation just uses their __dict__ directly.

This is backwards compatible, will just raise fewer exceptions in naive user code.

Originally implemented and reviewed at python/typing#470.

ambv pushed a commit to python/cpython that referenced this pull request

Sep 14, 2017

…nd modules (GH-3582) (#3583)

This makes the default behavior (without specifying globalns manually) more predictable for users, finds the right globalns automatically.

Implementation for classes assumes has a __module__ attribute and that module is present in sys.modules. It does this recursively for all bases in the MRO. For modules, the implementation just uses their __dict__ directly.

This is backwards compatible, will just raise fewer exceptions in naive user code.

Originally implemented and reviewed at python/typing#470. (cherry picked from commit f350a26)