Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.3k
gh-85795: Add support forsuper() inNamedTuple subclasses#129352
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 ourterms of service andprivacy 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
This needs a backport to 3.12 and 3.13. |
bswck commentedJan 28, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Changed my mind, let's not backport it :)
|
bswck commentedJan 28, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
CPython disallows classFoo:__classcell__=None but this implementation allows fromtypingimportNamedTupleclassFoo(NamedTuple):__classcell__=None should I pop from the origin class namespace with a sentinel object fallback instead of Errors should never pass silently, unless explicitly silenced. |
Uh oh!
There was an error while loading.Please reload this page.
Ideally I feel like this fix would not involve adding a new parameter to
It seems like an unlikely situation, but I suppose this would be strictly-speaking more correct! |
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.
Mostly LGTM, but I have a few thoughts.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
bswck commentedJan 28, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I like@ZeroIntensity's idea of moving |
20bfaeb to074df4cCompareThere 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.
Nice. This LGTM now.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
oh, except some tests are failing |
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
0d6dcbb toee93519Comparebswck commentedJan 29, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hah! How ignorant of me. I added another frame by wrapping the function and didn't fixup the existing frame refs ( |
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.
LGTM, with one tiny suggestion.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
bswck commentedJan 29, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks@AlexWaygood and@ZeroIntensity for working through this and finding a better approach! One last thought is whether I should test cpython/Lib/test/test_super.py Lines 262 to 273 inc931d75
I'm leaving this decision to@rhettinger since he self-assigned this. I personally believe a possible regression allowing nonsense overwrites of __classcell__ intyping.NamedTuple subclasses is not harmful by any means and niche enough that and I wouldn't care about that case.@rhettinger Please review. :) |
rhettinger commentedFeb 6, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I don't think this is worth it and instead prefer Alex's suggestion "to document that super() doesn't work inside methods on typing.NamedTuples created using the class syntax". Named tuples are thin classes, there isn't much in the way of reusable code. Likely, that is why overriding rather than extending is the norm. Also dataclasses seem to be preferred when people are trying to do anything more interesting. I do admire the analysis and effort that was put into this PR. The code looks correct. However, I think almost no one needs this. The edit is a really interesting (and brilliant) but slightly icky magic trick that isn't that easy to explain. Neither FWIW, I'm only |
bswck commentedFeb 6, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@rhettinger Thank you. I must say I agree with you for the most part, I also don't feel too strong about the general necessity for this to work, especially since How about I make a PR to detect classcells and raise a proper, easy to comprehend error message like "using |
+1 That would tremendously improve the user experience. |
I'm therefore closing this PR and will create another one with the improved error message. Thank y'all for reviews, this bug was very fun to work on, so no regrets though it didn't make a cut :) |
Uh oh!
There was an error while loading.Please reload this page.
Fixesgh-85795.A private parameter
_classcellwas added tocollections.namedtuple()and its wrappers intypingin order to deliver it to the targetedtype.__new__.It's the easiest approach here.
__classcell__ispopped from the original class namespace, as it would otherwise leak to the end class (metaclasses should clean up the__classcell__attribute).It is expected not to be added to typeshed, as the parameter is intended for internal use only.
After this PR, please consider a follow-up issuegh-129343 and the PR associated with it.
📚 Documentation preview 📚:https://cpython-previews--129352.org.readthedocs.build/