Skip to content

bpo-46120: State that | is preferred over Union#30222

Merged
gpshead merged 2 commits intopython:mainfrom
sobolevn:issue-46120
Dec 24, 2021
Merged

bpo-46120: State that | is preferred over Union#30222
gpshead merged 2 commits intopython:mainfrom
sobolevn:issue-46120

Conversation

@sobolevn
Copy link
Copy Markdown
Member

@sobolevn sobolevn commented Dec 21, 2021

Comment thread Doc/library/typing.rst Outdated
Comment thread Doc/library/typing.rst Outdated
Comment thread Misc/NEWS.d/next/Documentation/2021-12-21-12-45-57.bpo-46120.PE0DmJ.rst Outdated
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.

Thanks Nikita!

Comment thread Doc/library/typing.rst Outdated
Copy link
Copy Markdown
Member

@Fidget-Spinner Fidget-Spinner Dec 21, 2021

Choose a reason for hiding this comment

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

I think Optional was still being debated. Should we leave it out for now?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

My opinion is that X | None is better than Optional:

  1. It does not create a separate concept. It is just a union with None
  2. It does not "look" like "optional argument" to some users

But, this is not a strong opinion. So, I can do both 🙂

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I also prefer X | None over Optional

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Optional is also more problematic than Union, because it's a constant cause for confusion.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You could frame this to avoid adding too much opinion:

``X | None`` is equivalent to ``Optional[X]`` and ``Union[X, None]``.

@AlexWaygood AlexWaygood self-requested a review December 21, 2021 10:18
Comment thread Doc/library/typing.rst Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I wonder if this might be confusing, we are using the two styles here for unions 🤔

could we do something like:

IntOrStr = int | str

IntOrStr | float == int | str | float

?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good idea. But, I guess we cannot completely remove Union[Union[]] example, because it is important as well. Maybe we can show both?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

why not :) hopefully it doesn’t get too verbose though

Comment thread Doc/library/typing.rst Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

is it worth mentioning that Union and | behave a bit differently when used at runtime?

>>> type(Union[str, int])
<class 'typing._UnionGenericAlias'>
>>> type(str | int)
<class 'types.UnionType'>

Copy link
Copy Markdown
Member Author

@sobolevn sobolevn Dec 21, 2021

Choose a reason for hiding this comment

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

yes, I think that works ☺️

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

While they're technically equivalent (and will evaluate as equal), they're not identical. Would it be worth hinting at this by saying they're logically equivalent?

@sobolevn sobolevn changed the title bpo-46120: State that | is prefered over Union bpo-46120: State that | is preferred over Union Dec 21, 2021
Comment thread Doc/library/typing.rst Outdated
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.

wording: "is preferred for readability" using "for" not "in". (same in the NEWS entry)

Comment thread Doc/library/typing.rst Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor suggestion:

``X | None`` is logically equivalent to ``Optional[X]`` and ``Union[X, None]``.

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.

I prefer "semantically equivalent" to "logically equivalent" 😉

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Even better.

Comment thread Doc/library/typing.rst Outdated
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.

arguments are only present when using the Union[X, Y] syntax. Prepend a clarification on this "When using Union[] syntax, the arguments ..."

Add something that covers what may be |ed together. "Only types may be used with the | operator for this purpose" or similar perhaps.

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.

This text is technically in the "module contents" section. There is more on X|Y in the docs for the types module. Maybe we could link there (on the word "shorthand") like was done in the first version of this PR. If the description of X|Y there is not clear enough it should be updated there IMO.

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.

Crosslinking shorthand does make sense. Otherwise I think the text here is clear enough as is.

Comment thread Doc/library/typing.rst Outdated
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.

is this linking to https://docs.python.org/3.11/library/stdtypes.html#types-union ?

The wording there doesn't specifically cover Union vs | behavior differences.

@bedevere-bot
Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

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

I would honestly start over -- throw away all changes so far, and take this:

To define a union, use e.g. ``Union[int, str]`` or the shorthand ``int | str``. Details:

Insert a single sentence before "Details:"

Using that shorthand is recommended.

@sobolevn
Copy link
Copy Markdown
Member Author

@gvanrossum, sure! Thank you for the feedback.

I have made the requested changes; please review again

@bedevere-bot
Copy link
Copy Markdown

Thanks for making the requested changes!

@gpshead: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from gpshead December 22, 2021 19:06
Comment thread Misc/NEWS.d/next/Documentation/2021-12-21-12-45-57.bpo-46120.PE0DmJ.rst Outdated
@gvanrossum
Copy link
Copy Markdown
Member

@gpshead It’s all in. Can you merge?

@gpshead gpshead merged commit 1b30660 into python:main Dec 24, 2021
@gpshead
Copy link
Copy Markdown
Member

gpshead commented Dec 24, 2021

is this a 3.10 or 3.11 thing. (apply the 3.10 backport label if needed to trigger that if so)

@gvanrossum gvanrossum added the needs backport to 3.10 only security fixes label Dec 24, 2021
@miss-islington
Copy link
Copy Markdown
Contributor

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

@bedevere-bot
Copy link
Copy Markdown

GH-30250 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 Dec 24, 2021
@gvanrossum
Copy link
Copy Markdown
Member

Definitely 3.10. Can someone check if 3.9 has ‘|’ for unions?

@AlexWaygood
Copy link
Copy Markdown
Member

Definitely 3.10. Can someone check if 3.9 has ‘|’ for unions?

It does not

ambv pushed a commit to miss-islington/cpython that referenced this pull request Dec 29, 2021
Co-authored-by: Éric <[email protected]>
Co-authored-by: Guido van Rossum <[email protected]>
(cherry picked from commit 1b30660)

Co-authored-by: Nikita Sobolev <[email protected]>
miss-islington added a commit that referenced this pull request Dec 29, 2021
…GH-30250)

Co-authored-by: Éric <[email protected]>
Co-authored-by: Guido van Rossum <[email protected]>
(cherry picked from commit 1b30660)


Co-authored-by: Nikita Sobolev <[email protected]>

Automerge-Triggered-By: GH:gpshead
@AlexWaygood AlexWaygood removed their request for review June 5, 2022 11:04
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.