Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-104549: Set __module__ on TypeAliasType#104550
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
AlexWaygood left a comment
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 can't review the C code, but I much prefer this behaviour to the behaviour on main! Manual testing also didn't reveal any bugs.
Maybe it's also worth testing that TypeAliasType.__module__ == 'typing'? (I.e., accessing __module__ on the class itself rather than instances of the class.) That's the only comment I have!
Eclips4 commented May 16, 2023
One nit about tests: may it be useful to define a type alias in another module, and check something like |
JelleZijlstra commented May 17, 2023
@AlexWaygood unfortunately this no longer works, I get This seems to be an inescapable consequence of how heap types work ( Line 1045 in 1b5a2b0
@Eclips4 good call about tests in another module, I'll add some. As for the module being NULL, apparently that can happen in various exotic situations where there is no calling function or that function has no module. Not 100% sure how to reproduce that. |
AlexWaygood commented May 17, 2023
That's a shame :( This inconsistency is a little unfortunate: >>> from typing import TypeAliasType, TypeVar >>> TypeAliasType.__module__ <attribute '__module__' of 'typing.TypeAliasType' objects> >>> TypeVar.__module__ 'typing' >>> type T =int|str >>> T.__module__ '__main__' >>> TypeVar("T").__module__ '__main__' |
AlexWaygood commented May 17, 2023
(But if there's no way of resolving that, then this is still definitely an improvement on the status quo :) |
JelleZijlstra commented May 17, 2023
Possible solutions:
|
AlexWaygood commented May 17, 2023
Yeah, I agree that that allowing arbitrary setting of attributes seems unnecessary |
JelleZijlstra commented May 18, 2023
oops, messed up the merge |
Uh oh!
There was an error while loading. Please reload this page.
bedevere-bot commented May 18, 2023
carljm left a comment
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.
Assuming the refleak buildbots are happy, this LGTM.
* main: pythongh-74690: Don't set special protocol attributes on non-protocol subclasses of protocols (python#104622) pythongh-104623: Update Windows installer to use SQLite 3.42.0 (python#104625) pythongh-104050: Add more type annotations to Argument Clinic (python#104628) pythongh-104629: Don't skip test_clinic if _testclinic is missing (python#104630) pythongh-104549: Set __module__ on TypeAliasType (python#104550) pythongh-104050: Improve some typing around `default`s and sentinel values (python#104626) pythongh-104146: Remove unused vars from Argument Clinic (python#104627) pythongh-104615: don't make unsafe swaps in apply_static_swaps (python#104620) pythonGH-104484: Add case_sensitive argument to `pathlib.PurePath.match()` (pythonGH-104565) pythonGH-96803: Document and test new unstable internal frame API functions (pythonGH-104211) pythonGH-104580: Don't cache eval breaker in interpreter (pythonGH-104581)
Uh oh!
There was an error while loading. Please reload this page.