Skip to content

bpo-43766: Implement PEP 647 (User-Defined Type Guards) in typing.py #25282

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 13 commits into from
Apr 27, 2021

Conversation

Fidget-Spinner
Copy link
Member

@Fidget-Spinner Fidget-Spinner commented Apr 8, 2021

@Fidget-Spinner Fidget-Spinner changed the title Implement PEP 647 (User-Defined Type Guards) in typing.py bpo-43766: Implement PEP 647 (User-Defined Type Guards) in typing.py Apr 8, 2021
@gvanrossum gvanrossum removed the request for review from ilevkivskyi April 10, 2021 20:13
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Thank you again! I have a bunch of editorial nits.

@Fidget-Spinner
Copy link
Member Author

Wow, you're on a roll with the typing reviews today! Thanks for taking the time to review the docs Guido!

Re: the comments about runtime mishaps, here's a snippet from lower down in the docs:

   .. note::

      Strict type narrowing is not enforced - ``TypeB`` need not be a narrower
      form of ``TypeA`` (it can even be a wider form) and this may lead to
      type-unsafe results.  The main reason is to allow for things like
      narrowing ``List[object]`` to ``List[str]`` which would fail under strict
      narrowing as ``List`` is invariant.  The responsibility of
      writing type-safe type guards is left to the user.

Do you think The responsibility of writing type-safe type guards is left to the user. is enough, or do you want me to expound more?

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Somehow GitHub did not make it possible to see just the changes since my last review. :-(

@gvanrossum
Copy link
Member

Wow, you're on a roll with the typing reviews today! Thanks for taking the time to review the docs Guido!

Catching up. :-)

Are there any other reviews I still owe you? I frankly have lost track.

Re: the comments about runtime mishaps, here's a snippet from lower down in the docs:

   .. note::

      Strict type narrowing is not enforced - ``TypeB`` need not be a narrower
      form of ``TypeA`` (it can even be a wider form) and this may lead to
      type-unsafe results.  The main reason is to allow for things like
      narrowing ``List[object]`` to ``List[str]`` which would fail under strict
      narrowing as ``List`` is invariant.  The responsibility of
      writing type-safe type guards is left to the user.

Heh, I nearly completely rewrote that in my last review.

Do you think The responsibility of writing type-safe type guards is left to the user. is enough, or do you want me to expound more?

That part I think is just fine!

@Fidget-Spinner
Copy link
Member Author

Fidget-Spinner commented Apr 11, 2021

Are there any other reviews I still owe you? I frankly have lost track.

Technically one more: #24056 . However, please hold off on that. I've recently been internally bikeshedding on whether to convert the entire PEP 612 implementation into pure Python to reduce maintenance burden and complexity in types.GenericAlias. After I went to read the PEP again, I have some ideas for collections.abc.Callable which may make that possible. I'll update you on bpo if my efforts come to fruition.

If what I plan for works, we won't need that PR anymore. And we'll be able to revert the C code to what it was before PEP 612 - a big plus in my opinion.

@Fidget-Spinner
Copy link
Member Author

Personally, I think we can merge this. IMO the implementation is 100% there. The docs look alright too (I've re-read it a bunch of times now). And we can always fix the docs in later update.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Okay, will merge.

@gvanrossum gvanrossum merged commit 05ab4b6 into python:master Apr 27, 2021
@bedevere-bot
Copy link

@gvanrossum: Please replace # with GH- in the commit message next time. Thanks!

@Fidget-Spinner
Copy link
Member Author

Sorry Guido, this PR broke doc builds on master because it triggers false positives in suspicious.py (which was re-enabled just recently). I've created #25660 to address your docs comments and fix the build errors.

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

Successfully merging this pull request may close these issues.

5 participants