-
-
Notifications
You must be signed in to change notification settings - Fork 32.6k
bpo-44353: Refactor typing.NewType into callable class #27250
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
Conversation
def __init__(self, name, tp): | ||
self.__name__ = name | ||
self.__qualname__ = name | ||
self.__module__ = _callee(default='typing') |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
GH-27258 is a backport of this pull request to the 3.10 branch. |
(cherry picked from commit 965dd76) Co-authored-by: Yurii Karabas <1998uriyyo@gmail.com>
@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. |
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)) |
There was a problem hiding this comment.
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__
.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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)
@Fidget-Spinner I notice that |
@uriyyo Good point! If you add a PR with such tests, I'll merge it. |
* 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
https://bugs.python.org/issue44353