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

Standardize Type[Union[...]] -> Union[Type[...]] #3209

Merged
merged 7 commits into from Jun 9, 2017

Conversation

Projects
None yet
3 participants
@pkch
Contributor

pkch commented Apr 21, 2017

This is the last commit from my #3005 PR that was just merged. @ilevkivskyi asked me to separate that commit into a standalone PR, see this discussion. Essentially, @ilevkivskyi asked me to check if Type[Union[A, B]] is handled correctly (that is, as equivalent to Union[Type[A], Type[B]]) in my PR. I realized that it's not, but it's also not handled correctly in existing code base (see here, for example).

One way to fix it, without adding a lot of new logic to the code, is to use Union[Type[A], Type[B]] internally to represent Type[Union[A, B]]. Of course, we'll need to make sure it always happens. One safe way to do that is what I've done in this PR:

  • Add static method TypeType.make that accepts the same arguments as TypeType constructor, but can return a UnionType if appropriate (i.e., if passed a UnionType as an item)
  • Replace all uses of constructor TypeType() with TypeType.make()
  • Disable TypeType() constructor

The potential alternative would be to:

  • Rename class TypeType into _TypeType
  • Create a function TypeType() that does precisely what my TypeType.make does
  • Fix the resulting typing issues (TypeType() function, while roughly equivalent to TypeType class, isn't quite the same because we treat classes and Callables differently)

Also in this PR:

  • Add tests for Type[Union[...]]
  • Update testTypeUsingTypeCClassMethodUnion, since it can now use the correct expect output
Standardize Type[Union...]] -> Union[Type[...]]
* Add more tests for Type[Union[...]]
* Force TypeType.make() to be used everywhere instead of TypeType()
* Accidentally fixed testTypeUsingTypeCClassMethodUnion

@ilevkivskyi ilevkivskyi changed the title from Standardize Type[Union...]] -> Union[Type[...]] to Standardize Type[Union[...]] -> Union[Type[...]] Apr 26, 2017

@ilevkivskyi

I like the idea of normalizing to a canonical representation, and I like how it is done (apart from one minor comment). This is quite significant change, so that I would like to ask @JukkaL if it is OK.

Show outdated Hide outdated mypy/types.py Outdated

@gvanrossum gvanrossum requested a review from JukkaL May 10, 2017

@JukkaL JukkaL self-assigned this May 12, 2017

@JukkaL

This looks a reasonable thing to do, but the implementation is a bit over-complicated and I'd like to see it simplified (see comments). Also can you fix the merge conflict?

Show outdated Hide outdated mypy/types.py Outdated
Show outdated Hide outdated mypy/types.py Outdated
Show outdated Hide outdated mypy/types.py Outdated
@pkch

This comment has been minimized.

Show comment
Hide comment
@pkch

pkch Jun 1, 2017

Contributor

I fixed the merge conflict, and will try to address your other comments soon.

Contributor

pkch commented Jun 1, 2017

I fixed the merge conflict, and will try to address your other comments soon.

pkch added some commits Jun 1, 2017

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Jun 3, 2017

Collaborator

@pkch
It looks like there is an unnecessary typeshed update in this PR now.

Collaborator

ilevkivskyi commented Jun 3, 2017

@pkch
It looks like there is an unnecessary typeshed update in this PR now.

@pkch

This comment has been minimized.

Show comment
Hide comment
@pkch

pkch Jun 3, 2017

Contributor

@ilevkivskyi ah right. fixed.

Contributor

pkch commented Jun 3, 2017

@ilevkivskyi ah right. fixed.

@pkch

This comment has been minimized.

Show comment
Hide comment
@pkch

pkch Jun 8, 2017

Contributor

@JukkaL @ilevkivskyi I incorporated all the comments so far, so when you get a chance can you look again?

Contributor

pkch commented Jun 8, 2017

@JukkaL @ilevkivskyi I incorporated all the comments so far, so when you get a chance can you look again?

@ilevkivskyi

Looks good to me. I leave this up to Jukka to merge.

@JukkaL

JukkaL approved these changes Jun 9, 2017

LGTM

@JukkaL JukkaL merged commit 051bba0 into python:master Jun 9, 2017

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