Skip to content

bpo-44957: Promote PEP 604 syntax in typing docs#27833

Merged
ambv merged 2 commits into
python:mainfrom
srittau:pep-604-optional
Aug 22, 2021
Merged

bpo-44957: Promote PEP 604 syntax in typing docs#27833
ambv merged 2 commits into
python:mainfrom
srittau:pep-604-optional

Conversation

@srittau
Copy link
Copy Markdown
Contributor

@srittau srittau commented Aug 19, 2021

  • Use "X | Y" instead of "Union" where it makes sense.
  • Mention that "X | Y" is equivalent to "Union[X, Y]" in Union section.
  • Remove "Optional[X]" as shorthand for "Union[X, None]" as the new
    shorthand is now "X | None".
  • Mention that "Optional[X]" can be written as "X | None" in section
    about "Optional".

https://bugs.python.org/issue44957

* Use "X | Y" instead of "Union" where it makes sense.
* Mention that "X | Y" is equivalent to "Union[X, Y]" in Union section.
* Remove "Optional[X]" as shorthand for "Union[X, None]" as the new
  shorthand is now "X | None".
* Mention that "Optional[X]" can be written as "X | None" in section
  about "Optional".
@srittau
Copy link
Copy Markdown
Contributor Author

srittau commented Aug 19, 2021

This should be backported to Python 3.10 (where PEP 604 was implemented), but not further.

@Fidget-Spinner Fidget-Spinner added the needs backport to 3.10 only security fixes label Aug 19, 2021
Copy link
Copy Markdown
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks Sebastian!

PS, I feel that the news item is optional, so if you don't want to include it, I'll be happy to apply skip news. However you're free to leave it in too.

Comment thread Doc/library/typing.rst
* Redundant arguments are skipped, e.g.::

Union[int, str, int] == Union[int, str]
Union[int, str, int] == Union[int, str] == int | str
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.

Not a critique and slightly off topic: this threw me off a little bit, because in earlier versions of 3.10 this was false because they are different types at runtime. Only since rc1 this is true.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Interesting, I didn't know that.

Copy link
Copy Markdown
Member

@Fidget-Spinner Fidget-Spinner Aug 19, 2021

Choose a reason for hiding this comment

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

Sorry for the misinformation, but I was wrong 😉 . I think my memory is getting hazy, it was hash(int | str) that wasn't equivalent to hash(Union[int, str]) up till rc1. Equality was there from day 1.

Comment thread Doc/library/typing.rst Outdated
Comment thread Doc/library/typing.rst
* You cannot subclass or instantiate a union.
* You cannot subclass or instantiate a ``Union``.

* You cannot write ``Union[X][Y]``.
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.

Oh I just noticed this and maybe this can be another PR (since it needs to be backported to 3.9) but isn't this inaccurate if X is a TypeVar?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

At least the sentence doesn't sound super relevant to me. All examples show how to use Union, so would someone really want to write Union[X][Y]? Maybe this is some historic discussion.

Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
@srittau
Copy link
Copy Markdown
Contributor Author

srittau commented Aug 19, 2021

PS, I feel that the news item is optional, so if you don't want to include it, I'll be happy to apply skip news. However you're free to leave it in too.

I added it, since this is a bit more than just a fix, but I have no strong opinions either way.

Copy link
Copy Markdown
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

Alright. I just have one more minor point left for discussion.

Comment thread Doc/library/typing.rst
To define a union, use e.g. ``Union[int, str]``. Details:
To define a union, use e.g. ``Union[int, str]`` or the shorthand ``int | str``. Details:

* The arguments must be types and there must be at least one.
Copy link
Copy Markdown
Member

@Fidget-Spinner Fidget-Spinner Aug 19, 2021

Choose a reason for hiding this comment

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

Out of curiosity, why did you change the redundant arguments are skipped bullet point, but not the others? I can see that Unions of a single argument vanish may not apply directly to the new syntax, but the others do right?

If we manage to merge them, then perhaps we can shorten the contents of the union_object == other section here https://docs.python.org/3.10/library/stdtypes.html#types-union and just link to the typing version. That will save us from some repetition.

Copy link
Copy Markdown
Contributor Author

@srittau srittau Aug 19, 2021

Choose a reason for hiding this comment

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

I went a bit back and forth over this. In the end I decided that most of them apply mostly to Union:

  • Unions of unions don't really exist for PEP 604 unions, except when counting (X | Y) | Z. So we could write one of the following:
    • Union[Union[int, str], float] == Union[int, str, float] == int | str | float
    • Union[Union[int, str], float] == int | str | float
      I kind of like the second one, since it emphasizes the new syntax, but I thought it might be a bit confusing.
  • The redundant arguments should probably be changed to either list both Union[int, str, int] == Union[int, str] and int | str | int == int | str or only the latter (my preference). No idea why I chose this strange middle thing.
  • Same of comparing unions, really.

Copy link
Copy Markdown
Member

@Fidget-Spinner Fidget-Spinner Aug 19, 2021

Choose a reason for hiding this comment

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

You points make sense. I would like to not outright replace the examples under typing.Union with only the PEP 604 version. For the same reason that we haven't changed examples under typing.List [1] to PEP 585 style list and instead just changed every other use of typing.List elsewhere (same for every other PEP 585-ied type really). Considering at runtime they aren't the same, I'd hate if beginners tripped over some runtime corner case thinking they were. (e.g. isinstance(1, int | str) works, isinstance(1, Union[int, str]) fails before 3.10)

This is one tough pickle :(. So I'm OK with the status quo.

[1] https://docs.python.org/3.11/library/typing.html#typing.List

@ambv ambv merged commit dabb6e8 into python:main Aug 22, 2021
@miss-islington
Copy link
Copy Markdown
Contributor

Thanks @srittau for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 22, 2021
* Use "X | Y" instead of "Union" where it makes sense.
* Mention that "X | Y" is equivalent to "Union[X, Y]" in Union section.
* Remove "Optional[X]" as shorthand for "Union[X, None]" as the new
  shorthand is now "X | None".
* Mention that "Optional[X]" can be written as "X | None" in section
  about "Optional".

Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
(cherry picked from commit dabb6e8)

Co-authored-by: Sebastian Rittau <srittau@rittau.biz>
@bedevere-bot
Copy link
Copy Markdown

GH-27897 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Aug 22, 2021
@srittau srittau deleted the pep-604-optional branch August 22, 2021 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Documentation in the Doc dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants