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

bpo-38364: unwrap partialmethods just like we unwrap partials #16600

Merged
merged 5 commits into from
Feb 15, 2024

Conversation

mjpieters
Copy link
Contributor

@mjpieters mjpieters commented Oct 5, 2019

The inspect.isgeneratorfunction, inspect.iscoroutinefunction and inspect.isasyncgenfunction already unwrap functools.partial objects, this patch adds support for partialmethod objects as well.

https://bugs.python.org/issue38364

Lib/test/test_inspect.py Outdated Show resolved Hide resolved
@florimondmanca
Copy link

florimondmanca commented Dec 21, 2019

Coincidently, this would also resolve another situation, i.e. when passing the partial of a method directly…

import functools, inspect

class Dog:
    async def abark(self): pass

inspect.iscoroutinefunction(dog.abark)  # True
inspect.iscoroutinefunction(functools.partial(dog.abark))  # False (should probably be True?)

This would be because the partial would be unwrapped (via the inner call to _unwrap_partial() here), yielding a method object that will be correctly handled by the method unwrapping.

@jalaziz
Copy link

jalaziz commented Mar 5, 2022

This is also an issue for unittest mocking. patch will return MagicMock for partial async methods and not AsyncMock. In our case, the underlying code is using calling partial against a method, but as @florimondmanca pointed out, this PR should fix it.

Copy link
Contributor

@kumaraditya303 kumaraditya303 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are merge conflicts.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@mjpieters mjpieters force-pushed the bpo38364_partialmethod_inspect branch from 03ca24c to 84dd702 Compare December 30, 2022 14:14
@mjpieters
Copy link
Contributor Author

I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@kumaraditya303: please review the changes made to this pull request.

Doc/library/inspect.rst Outdated Show resolved Hide resolved
Doc/library/inspect.rst Outdated Show resolved Hide resolved
Lib/functools.py Outdated
prev = None
while func is not prev:
prev = func
while isinstance(getattr(func, "_partialmethod", None), partialmethod):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why getattr? What objects have _partialmethod?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

partialmethod(...).__get__() can return a closure with _partialmethod pointing to the instance. This happens if the wrapped callable is not a descriptor, or if the descriptor returned itself again.

See the partialmethod._make_unbound_method() implementation.

getattr() is used because the attribute is optional, and you'd not want this to break even if the supplied object does have a _partialmethod attribute that is not actually a partialmethod instance.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@mjpieters mjpieters force-pushed the bpo38364_partialmethod_inspect branch from 35b8c19 to 2fb40c2 Compare December 31, 2022 11:30
@mjpieters
Copy link
Contributor Author

I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@kumaraditya303: please review the changes made to this pull request.

The inspect.isgeneratorfunction, inspect.iscoroutinefunction and inspect.isasyncgenfunction already unwrap functools.partial objects, this patch adds support for partialmethod objects as well.
@mjpieters mjpieters force-pushed the bpo38364_partialmethod_inspect branch from 2fb40c2 to 243cb00 Compare December 31, 2022 11:35
@kumaraditya303 kumaraditya303 removed their request for review May 30, 2023 12:31
Since we'rs checking this attribute on arbitrary function-like objects,
we should use the namespace reserved for core Python.
@encukou
Copy link
Member

encukou commented Jan 25, 2024

Hi! Just getting to this, sorry for the delay.
Since we're accessing _partialmethod on arbitrary objects, it would be best to rename it to __partialmethod__ -- make it part of the namespace reserved for Python.

I hope you don't mind me pushing directly to the PR.

@encukou encukou dismissed kumaraditya303’s stale review February 1, 2024 12:53

All concerns from the review should be resolved now.

@encukou encukou merged commit edb59d5 into python:main Feb 15, 2024
30 of 31 checks passed
@mjpieters mjpieters deleted the bpo38364_partialmethod_inspect branch February 20, 2024 12:57
rominf added a commit to rominf/sentry-python that referenced this pull request Jul 10, 2024
…on 3.13)

The `_partialmethod` attribute of methods wrapped with `partialmethod()`
was renamed to `__partialmethod__` in CPython 3.13:
python/cpython#16600
rominf added a commit to rominf/sentry-python that referenced this pull request Jul 10, 2024
…on 3.13)

The `_partialmethod` attribute of methods wrapped with `partialmethod()`
was renamed to `__partialmethod__` in CPython 3.13:
python/cpython#16600
sentrivana pushed a commit to getsentry/sentry-python that referenced this pull request Jul 16, 2024
…on 3.13) (#3272)

The `_partialmethod` attribute of methods wrapped with `partialmethod()`
was renamed to `__partialmethod__` in CPython 3.13:
python/cpython#16600
sentrivana added a commit to getsentry/sentry-python that referenced this pull request Jul 16, 2024
Adding preliminary support for Python 3.13.

The `_partialmethod` attribute of methods wrapped with `partialmethod()`
was renamed to `__partialmethod__` in CPython 3.13:
python/cpython#16600

Starting from Python 3.13, `frame.f_locals` is not `dict` anymore, but
`FrameLocalsProxy`, that cannot be copied using `copy.copy()`. In Python
3.13 and later, it should be copied using a method `.copy()`. The new way
of copying works the same as the old one for versions of Python prior to
3.13, according to the documentation (both copying methods produce a
shallow copy).

Since Python 3.13, `FrameLocalsProxy` skips items of `locals()` that have
non-`str` keys; this is a CPython implementation detail, so we hence
disable `test_non_string_variables` test on Python 3.13.

See:
https://peps.python.org/pep-0667/
python/cpython#118921
python/cpython#118923
https://docs.python.org/3.13/whatsnew/3.13.html#porting-to-python-3-13
https://docs.python.org/3/library/copy.html
https:/python/cpython/blame/7b413952e817ae87bfda2ac85dd84d30a6ce743b/Objects/frameobject.c#L148

---------

Co-authored-by: Roman Inflianskas <[email protected]>
arjennienhuis pushed a commit to arjennienhuis/sentry-python that referenced this pull request Sep 30, 2024
Adding preliminary support for Python 3.13.

The `_partialmethod` attribute of methods wrapped with `partialmethod()`
was renamed to `__partialmethod__` in CPython 3.13:
python/cpython#16600

Starting from Python 3.13, `frame.f_locals` is not `dict` anymore, but
`FrameLocalsProxy`, that cannot be copied using `copy.copy()`. In Python
3.13 and later, it should be copied using a method `.copy()`. The new way
of copying works the same as the old one for versions of Python prior to
3.13, according to the documentation (both copying methods produce a
shallow copy).

Since Python 3.13, `FrameLocalsProxy` skips items of `locals()` that have
non-`str` keys; this is a CPython implementation detail, so we hence
disable `test_non_string_variables` test on Python 3.13.

See:
https://peps.python.org/pep-0667/
python/cpython#118921
python/cpython#118923
https://docs.python.org/3.13/whatsnew/3.13.html#porting-to-python-3-13
https://docs.python.org/3/library/copy.html
https:/python/cpython/blame/7b413952e817ae87bfda2ac85dd84d30a6ce743b/Objects/frameobject.c#L148

---------

Co-authored-by: Roman Inflianskas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants