◐ Shell
reader mode source ↗
Skip to content

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

Merged
gvanrossum merged 13 commits into
python:masterfrom
Fidget-Spinner:pep647
Apr 27, 2021
Merged

bpo-43766: Implement PEP 647 (User-Defined Type Guards) in typing.py#25282
gvanrossum merged 13 commits into
python:masterfrom
Fidget-Spinner:pep647

Conversation

@Fidget-Spinner

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

Copy link
Copy Markdown
Member

@Fidget-Spinner Fidget-Spinner changed the title 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

@gvanrossum gvanrossum left a comment

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.

Hide comment

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

5 hidden conversations Load more…
@Fidget-Spinner

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

@gvanrossum gvanrossum left a comment

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.

Hide comment

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

@gvanrossum

Copy link
Copy Markdown
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 and others added 3 commits April 11, 2021 11:23
Co-Authored-By: Guido van Rossum <gvanrossum@users.noreply.github.com>
@Fidget-Spinner

Fidget-Spinner commented Apr 11, 2021

Copy link
Copy Markdown
Member Author

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

@gvanrossum gvanrossum left a comment

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.

Hide comment

Okay, will merge.

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

Copy link
Copy Markdown

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

@bedevere-bot bedevere-bot removed the label Apr 27, 2021
@Fidget-Spinner Fidget-Spinner deleted the pep647 branch April 27, 2021 14:36
@Fidget-Spinner

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