Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

bpo-44353: Refactor typing.NewType into callable class#27250

Merged
ambv merged 1 commit into
python:mainpython/cpython:mainfrom
uriyyo:fix-issue-44353uriyyo/cpython:fix-issue-44353Copy head branch name to clipboard
Jul 20, 2021
Merged

bpo-44353: Refactor typing.NewType into callable class#27250
ambv merged 1 commit into
python:mainpython/cpython:mainfrom
uriyyo:fix-issue-44353uriyyo/cpython:fix-issue-44353Copy head branch name to clipboard

Conversation

@uriyyo

@uriyyo uriyyo commented Jul 19, 2021

Copy link
Copy Markdown
Member

Comment thread Lib/typing.py
def __init__(self, name, tp):
self.__name__ = name
self.__qualname__ = name
self.__module__ = _callee(default='typing')

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.

That makes it rather slow but I guess __module__ is actually helpful.

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.

It will be executed only once when NewType defined. I think it should not dramatically effect performance.

@ambv ambv merged commit 965dd76 into python:main Jul 20, 2021
@ambv ambv added the needs backport to 3.10 only security fixes label Jul 20, 2021
@miss-islington

Copy link
Copy Markdown
Contributor

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

@bedevere-bot

Copy link
Copy Markdown

GH-27258 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 Jul 20, 2021
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 20, 2021
(cherry picked from commit 965dd76)

Co-authored-by: Yurii Karabas <1998uriyyo@gmail.com>
@Fidget-Spinner

Copy link
Copy Markdown
Member

@ambv oh wow I wasn't expecting this to get merged yet, on bpo it was still being discussed https://bugs.python.org/issue44353#msg397842. I'm +0 for this personally, so if you're +1 I certainly won't complain.

Comment thread Lib/test/test_typing.py
Comment on lines +3697 to +3701
self.assertEqual(UserId | int, Union[UserId, int])
self.assertEqual(int | UserId, Union[int, UserId])

self.assertEqual(get_args(UserId | int), (UserId, int))
self.assertEqual(get_args(int | UserId), (int, UserId))

@Fidget-Spinner Fidget-Spinner Jul 20, 2021

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 think this doesn't test for the bug the user is reporting. In fact without this PR, these tests should all work, just instead of being typing.Union, it is types.Union (because int implements __or__). So the test won't be able to detect a regression.

The bug reported is that UserId1 | UserId2 won't work, because they both don't implement __or__.

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.

You are right, we should add tests to cover case when there are two NewType.

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.

I will open another PR with more tests. I was not expecting that it will be merged so fast)

@uriyyo

uriyyo commented Jul 23, 2021

Copy link
Copy Markdown
Member Author

@Fidget-Spinner I notice that NewType become pickable. Should we add tests to cover this?

@ambv

ambv commented Jul 23, 2021

Copy link
Copy Markdown
Contributor

@uriyyo Good point! If you add a PR with such tests, I'll merge it.

armoha added a commit to armoha/eudplib that referenced this pull request Nov 2, 2022
* Also added nice repr for consttype!
  - referred to python/cpython#27250
* TODO: add built-in sounds
  - in-game: sound/protoss/artanis/patpss01.wav
  - campaign: campaign\protoss\protoss04\staredit\wav\p4m02uta.ogg
  - localized: locales\koKR\Assets\campaign\Protoss\Protoss04\staredit\wav\p4m02uta.ogg
* Fixed typo ;p
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.

6 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.