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

gh-85795: Fix typing.NamedTuple __classcell__ bug, support overriding __repr__ easily #129344

Closed

Conversation

bswck
Copy link
Contributor

@bswck bswck commented Jan 27, 2025

Closes gh-85795
Closes gh-129343

I'm open to criticism, especially the documentation part.
I'm elaborating on the solution in further comments.

I'll add the news entry quickly.


📚 Documentation preview 📚: https://cpython-previews--129344.org.readthedocs.build/

@bswck bswck changed the title gh-85795: Fix __classcell__, support overriding __repr__ easily gh-85795: Fix typing.NamedTuple __classcell__ bug, support overriding __repr__ easily Jan 27, 2025
@bswck bswck force-pushed the fix-classcell-bug-and-overriding-repr branch from 96af8ac to dcdab5f Compare January 27, 2025 11:49
@bswck
Copy link
Contributor Author

bswck commented Jan 27, 2025

(Sorry for the force push, but this one was a very tricky and subtle git mistake of mine and I prefer not confusing y'all)

@bswck
Copy link
Contributor Author

bswck commented Jan 27, 2025

A couple notes on the solution:

I added a private parameter _classcell to collections.namedtuple() and its wrappers in typing in order to deliver it to the targeted type.__new__.
It's the easiest approach here.

I also pop __classcell__ from the original class namespace, as it would otherwise leak to the end class (metaclasses should clean up the __classcell__ attribute).

I expect it is not added to typeshed, as it is only intended for internal use.

I expect _repr added to typeshed though and offer a PR for it upon acceptance of this solution.

@bswck
Copy link
Contributor Author

bswck commented Jan 27, 2025

If you disagree with the addition of _repr to named tuples, we can split this PR. Just let me know.

@AlexWaygood
Copy link
Member

AlexWaygood commented Jan 27, 2025

If you disagree with the addition of _repr to named tuples, we can split this PR. Just let me know.

It would make sense to me to split this PR into two. I'm not necessarily saying I disagree with the _repr addition, but it seems less clearly a bugfix than the __classcell__ thing. Small, incremental PRs are also easier for us to review :-)

@bswck
Copy link
Contributor Author

bswck commented Jan 27, 2025

@AlexWaygood Thanks, you're right. I should have split that in the first place. I'll work through all the possible CI problems and then proceed to split it.

@bswck
Copy link
Contributor Author

bswck commented Jan 27, 2025

This looks super-bad!
image

@bswck
Copy link
Contributor Author

bswck commented Jan 27, 2025

Closing this one and opening two new PRs, as concluded in #129344 (comment).

@bswck bswck closed this Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add _repr to named tuples __class__ not set defining 'X' as <class '__main__.X'>
2 participants