Skip to content

add __class_getitem__ to the python implementation of functools.partial #127537

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

Merged

Conversation

cfbolz
Copy link
Contributor

@cfbolz cfbolz commented Dec 2, 2024

The Python version of functools.partial is missing the __class_getitem__ that would make it usable as a generic alias. This never matters for CPython, because it always uses the C-implementation of that class. However, in PyPy we always use the Python version.

The problem went unnoticed because the tests for this behaviour got added to test_genericalias.py, which is only testing the C version (unlike test_functools.py).

@rhettinger
Copy link
Contributor

This looks reasonable to me.

@picnixz
Copy link
Member

picnixz commented Dec 2, 2024

I think a NEWS entry is needed for this one. And an issue just for tracking the changes (or pick an issue that was about adding __class_getitem__ to standard types) (but otherwise I also think it makes sense).

@rhettinger
Copy link
Contributor

@JelleZijlstra If this is approved, is it a candidate for backporting?

@JelleZijlstra
Copy link
Member

When was the corresponding change made to the C version? I'd support backporting to any version where __class_getitem__ already worked on the C version.

@JelleZijlstra JelleZijlstra self-requested a review December 3, 2024 03:37
@cfbolz
Copy link
Contributor Author

cfbolz commented Dec 3, 2024

The feature got added as part of #83662, in this commit: cecf049673da. I'm happy to create a tracking issue and treat this as an (unobservable in CPython) bug, if people think that's actually useful.

I can also add a news entry but from the perspective of a CPython user this is an unobservable change.

@picnixz
Copy link
Member

picnixz commented Dec 3, 2024

I'm happy to create a tracking issue and

I think we can just use that issue instead.

CPython user this is an unobservable change.

Oh, I considered PyPy as being a CPython user in this sense. But maybe it makes sense not to create the NEWS actually (now that I think about it, I think it'd be more confusing than helping).

@JelleZijlstra
Copy link
Member

Thanks, looks like it got added in 3.9 already. So we should backport to all branches that still get bugfixes (3.12 and 3.13).

Yes, please add a NEWS entry. You can use the issue number from the original change or create a new issue.

@cfbolz
Copy link
Contributor Author

cfbolz commented Dec 4, 2024

Thanks, looks like it got added in 3.9 already.

I could not find it on the 3.9 branch. are you sure you didn't mix it up with partialmethod.__class_getitem__?

Anyway, I've added a news entry now.

@hauntsaninja hauntsaninja added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Dec 15, 2024
@hauntsaninja
Copy link
Contributor

partial_methods is just passed to the tp_methods of the functools.partial type, is not actually functools.partialmethod

@hauntsaninja hauntsaninja merged commit 401bba6 into python:main Dec 27, 2024
46 checks passed
@miss-islington-app
Copy link

Thanks @cfbolz for the PR, and @hauntsaninja for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 27, 2024
…f functools.partial (pythonGH-127537)

(cherry picked from commit 401bba6)

Co-authored-by: CF Bolz-Tereick <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Dec 27, 2024

GH-128281 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Dec 27, 2024
@miss-islington-app
Copy link

Sorry, @cfbolz and @hauntsaninja, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 401bba6b58497ce59e7b45ad33e43ae8c67abcb9 3.12

@bedevere-app
Copy link

bedevere-app bot commented Dec 27, 2024

GH-128282 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Dec 27, 2024
hauntsaninja pushed a commit to hauntsaninja/cpython that referenced this pull request Dec 27, 2024
hauntsaninja added a commit that referenced this pull request Dec 27, 2024
…of functools.partial (#127537) (#128282)

gh-127537: Add __class_getitem__ to the python implementation of functools.partial (#127537)

(cherry picked from commit 401bba6)

Co-authored-by: CF Bolz-Tereick <[email protected]>
hauntsaninja pushed a commit that referenced this pull request Dec 27, 2024
…of functools.partial (GH-127537) (#128281)

gh-127537: Add __class_getitem__ to the python implementation of functools.partial (GH-127537)
(cherry picked from commit 401bba6)

Co-authored-by: CF Bolz-Tereick <[email protected]>
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 8, 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.

5 participants