bpo-46120: State that | is preferred over Union#30222
Conversation
There was a problem hiding this comment.
I think Optional was still being debated. Should we leave it out for now?
There was a problem hiding this comment.
My opinion is that X | None is better than Optional:
- It does not create a separate concept. It is just a union with
None - It does not "look" like "optional argument" to some users
But, this is not a strong opinion. So, I can do both 🙂
There was a problem hiding this comment.
I also prefer X | None over Optional
There was a problem hiding this comment.
Optional is also more problematic than Union, because it's a constant cause for confusion.
There was a problem hiding this comment.
You could frame this to avoid adding too much opinion:
``X | None`` is equivalent to ``Optional[X]`` and ``Union[X, None]``.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Good idea. But, I guess we cannot completely remove Union[Union[]] example, because it is important as well. Maybe we can show both?
There was a problem hiding this comment.
why not :) hopefully it doesn’t get too verbose though
There was a problem hiding this comment.
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'>
There was a problem hiding this comment.
I can add a link to https://docs.python.org/3/library/stdtypes.html#union-type
There was a problem hiding this comment.
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?
| is prefered over Union| is preferred over Union
There was a problem hiding this comment.
wording: "is preferred for readability" using "for" not "in". (same in the NEWS entry)
There was a problem hiding this comment.
Minor suggestion:
``X | None`` is logically equivalent to ``Optional[X]`` and ``Union[X, None]``.
There was a problem hiding this comment.
I prefer "semantically equivalent" to "logically equivalent" 😉
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Crosslinking shorthand does make sense. Otherwise I think the text here is clear enough as is.
There was a problem hiding this comment.
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.
|
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 |
gvanrossum
left a comment
There was a problem hiding this comment.
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.
2d252ab to
f9d8689
Compare
|
@gvanrossum, sure! Thank you for the feedback. I have made the requested changes; please review again |
|
Thanks for making the requested changes! @gpshead: please review the changes made to this pull request. |
…E0DmJ.rst Co-authored-by: Éric <[email protected]>
|
@gpshead It’s all in. Can you merge? |
|
is this a 3.10 or 3.11 thing. (apply the 3.10 backport label if needed to trigger that if so) |
|
GH-30250 is a backport of this pull request to the 3.10 branch. |
|
Definitely 3.10. Can someone check if 3.9 has ‘|’ for unions? |
It does not |
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]>
…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
Originally discussed in https://mail.python.org/archives/list/[email protected]/thread/TW5M6XDN7DO346RXMADKBAAGNSZPC4ZQ/
CC @Fidget-Spinner
https://bugs.python.org/issue46120