Skip to content

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

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 2 commits into from
Dec 24, 2021

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Dec 21, 2021

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

@@ -661,6 +664,7 @@ These can be used as types in annotations using ``[]``, each having a unique syn
Optional type.

``Optional[X]`` is equivalent to ``X | None`` (or ``Union[X, None]``).
``X | None`` is prefered in readability in most cases.
Copy link
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
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
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
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
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

* The arguments must be types and there must be at least one.

* Unions of unions are flattened, e.g.::

Union[Union[int, str], float] == Union[int, str, float]
Union[Union[int, str], float] == int | str | float

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
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?

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


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

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

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
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

To define a union, use e.g. ``Union[int, str]`` or the shorthand ``int | str``. Details:
There's also an older version ``Union[int, str]``.
Note that the ``X | Y`` is preferred in readability in most cases.
Copy link
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)

@@ -660,7 +667,8 @@ These can be used as types in annotations using ``[]``, each having a unique syn

Optional type.

``Optional[X]`` is equivalent to ``X | None`` (or ``Union[X, None]``).
``Optional[X]`` is logically equivalent to ``X | None`` and ``Union[X, None]``.
Copy link
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
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
Contributor

Choose a reason for hiding this comment

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

Even better.

Note that the ``X | Y`` is preferred in readability in most cases.
However, there are :ref:`specified runtime differences<types-union>`.

Details:

* The arguments must be types and there must be at least one.
Copy link
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
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
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.

To define a union, use e.g. ``Union[int, str]`` or the shorthand ``int | str``. Details:
There's also an older version ``Union[int, str]``.
Note that the ``X | Y`` is preferred in readability in most cases.
However, there are :ref:`specified runtime differences<types-union>`.
Copy link
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

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
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
Member Author

@gvanrossum, sure! Thank you for the feedback.

I have made the requested changes; please review again

@bedevere-bot
Copy link

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
@gvanrossum
Copy link
Member

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

@gpshead gpshead merged commit 1b30660 into python:main Dec 24, 2021
@gpshead
Copy link
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
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

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
Member

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

@AlexWaygood
Copy link
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.