Skip to content
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

bpo-44732: Rename types.Union to types.UnionType #27342

Merged
merged 3 commits into from
Jul 26, 2021
Merged

bpo-44732: Rename types.Union to types.UnionType #27342

merged 3 commits into from
Jul 26, 2021

Conversation

AliyevH
Copy link
Contributor

@AliyevH AliyevH commented Jul 24, 2021

Copy link
Member

@FFY00 FFY00 left a comment

Choose a reason for hiding this comment

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

A plain rename from Union to UnionType would be a big breaking change, it should be deprecated IMO.

We could do that in types.__getattr__, by adding the following to the types module.

import warnings

...

def __getattr__(name):
    if name == 'Union':
        warnings.warn('types.Union is deprecated and should be replaced with types.UnionType, it will get removed in X time', DeprecationWarning)
        return UnionType
    raise AttributeError(f'module {__name__} has no attribute {name}')

@@ -1,2 +1,2 @@
Add ability to serialise ``types.Union`` objects. Patch provided by Yurii
Add ability to serialise ``types.UnionType`` objects. Patch provided by Yurii
Copy link
Member

Choose a reason for hiding this comment

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

We should not change this news entry 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't know about it 😕

Copy link
Member

@FFY00 FFY00 Jul 25, 2021

Choose a reason for hiding this comment

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

Oh wait, maybe we do actually want this changed as it hasn't been released yet 🤦. Sorry!

Copy link
Contributor Author

@AliyevH AliyevH Jul 25, 2021

Choose a reason for hiding this comment

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

no problem.
Can you please tell me X time in deprecation warning?😬

Copy link
Member

Choose a reason for hiding this comment

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

I don't know, that's up to the core devs, ask in https://bugs.python.org/issue44732 if we should have a deprecation period and if so, how long.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okey thanks 😄

@Fidget-Spinner
Copy link
Member

Fidget-Spinner commented Jul 25, 2021

A plain rename from Union to UnionType would be a big breaking change, it should be deprecated IMO.

From what I understand, it won't be. It looks like Serhiy is planning to backport the changes to 3.10 too where Union was first introduced. Hasan, please wait a bit to see what he has to say about the PR. Thanks!

@AliyevH
Copy link
Contributor Author

AliyevH commented Jul 25, 2021

Thanks @Fidget-Spinner . Okey let's wait 😃

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM!

@JelleZijlstra
Copy link
Member

I don't see a NEWS entry in the PR but bedevere passed, am I missing something?

I do think there should be a NEWS entry for this.

@gvanrossum
Copy link
Member

I don't see a NEWS entry in the PR but bedevere passed, am I missing something?

I do think there should be a NEWS entry for this.

The OP updated all of the existing NEWS entries that mention types.Union. That's creative, but I don't think it should be done that way. We need a new NEWS entry describing the renaming.

@AliyevH
Copy link
Contributor Author

AliyevH commented Jul 26, 2021

As i understand, we don't touch news files and add a new one with a description of renaming types.Union to types.UnionType.
I will update this today.

Lib/types.py Outdated
@@ -2,6 +2,7 @@
Define names for built-in types that aren't directly accessible as a builtin.
"""
import sys
import warnings
Copy link
Member

Choose a reason for hiding this comment

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

Do we need import warnings? I can not see the usage of this module at types.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No :) I forgot about it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Member

@uriyyo uriyyo left a comment

Choose a reason for hiding this comment

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

LGTM💫

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.

LGTM. Thanks for your contribution!

Minor note: in the future please don't force-push as it overrides commit history @AliyevH. Just commit normally, the core devs will squash and merge the PR.

https://devguide.python.org/pullrequest/#quick-guide

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.

Wait, the docs here need updating too https://docs.python.org/3.10/library/types.html#types.Union.

@AliyevH
Copy link
Contributor Author

AliyevH commented Jul 26, 2021

cs he

LGTM. Thanks for your contribution!

Minor note: in the future please don't force-push as it overrides commit history @AliyevH. Just commit normally, the core devs will squash and merge the PR.

https://devguide.python.org/pullrequest/#quick-guide

Got it. Thanks for support )

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@ambv ambv merged commit 2b8ad9e into python:main Jul 26, 2021
@bedevere-bot
Copy link

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

@miss-islington
Copy link
Contributor

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

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Jul 26, 2021
@bedevere-bot
Copy link

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 26, 2021
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
(cherry picked from commit 2b8ad9e)

Co-authored-by: Hasan <hasan.aleeyev@gmail.com>
miss-islington added a commit that referenced this pull request Jul 26, 2021
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
(cherry picked from commit 2b8ad9e)

Co-authored-by: Hasan <hasan.aleeyev@gmail.com>
tk0miya added a commit to tk0miya/sphinx that referenced this pull request Jul 29, 2021
Recently, `types.Union` was renamed to `types.UnionType` on the HEAD
of 3.10 (refs: python/cpython#27342). After this change, sphinx-build
has been crashed because of ImportError.
tk0miya added a commit to tk0miya/sphinx that referenced this pull request Jul 29, 2021
Recently, `types.Union` was renamed to `types.UnionType` on the HEAD
of 3.10 (refs: python/cpython#27342). After this change, sphinx-build
has been crashed because of ImportError.
tk0miya added a commit to sphinx-doc/sphinx that referenced this pull request Jul 29, 2021
Recently, `types.Union` was renamed to `types.UnionType` on the HEAD
of 3.10 (refs: python/cpython#27342). After this change, sphinx-build
has been crashed because of ImportError.
@orsenthil
Copy link
Member

As it is already referenced in this ticket, it broke (the stable version of )sphinx-build and multiple documentation projects are going to be affected by this change.

This is the trackback from sphinx-build command line.

  File "/Users/senthilx/venvs/cpython/bin/sphinx-quickstart", line 5, in <module>
    from sphinx.cmd.quickstart import main
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/senthilx/venvs/cpython/lib/python3.11/site-packages/sphinx/cmd/quickstart.py", line 38, in <module>
    from sphinx.util.console import bold, color_terminal, colorize, nocolor, red  # type: ignore
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/senthilx/venvs/cpython/lib/python3.11/site-packages/sphinx/util/__init__.py", line 41, in <module>
    from sphinx.util.typing import PathMatcher
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/senthilx/venvs/cpython/lib/python3.11/site-packages/sphinx/util/typing.py", line 37, in <module>
    from types import Union as types_Union
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ImportError: cannot import name 'Union' from 'types' (/Users/senthilx/git/cpython/Lib/types.py)

Sphinx has already fixed it in master (sphinx-doc/sphinx#9513) - The fix (aliasing to UnionType) doesn't seem great too.

If we get more complaints, I think, either CPython or Sphinx project need to address this by communication / guidance.

It was hard to track down this failure with the trace back.

@Fidget-Spinner
Copy link
Member

Fidget-Spinner commented Aug 12, 2021

@orsenthil that's sad to hear :(. Is there some better place to make such changes known to the wider community?

This change was discussed and agreed upon on python-dev, but that's centered around CPython so I don't think there's much visibility. It also doesn't get a What's New because it was a change during pre-release versions.

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.