bpo-34890: Make iscoroutinefunction, isgeneratorfunction and isasyncgenfunction work with functools.partial by pablogsal · Pull Request #9903 · python/cpython · GitHub

I have added some extra test cases for isasyncgenfunction and isasyncgen in test_iscoroutine for symmetry, but I am not sure if that is the place for it. Tell me if I should reallocate these tests in other place.

https://bugs.python.org/issue34890

…work with partial

Wouldn't it be better to do this:

while isinstance(object, functools.partial):
    object = object.func

? The benefit would be support of nested partials and the code will probably look less ugly.

Also, while you're here, please rename object to obj or o.

@1st1 Done in 41f7f60. Thanks for the review :)

Why don't we add an _unwrap_partial helper. Or.. should we maybe use inspect.unwrap() too?

Actually, let's not use inspect.unwrap(). Decorating generators and coroutines isn't that common.

Do we still want the helper?

I have done the refactor in 3b841ac

Please use module import (i.e. import functools).

Done in adb5db6. I did that originally to simplify the double functools.partial wrapping in the tests.

Hummmm, it seems that this changes the behaviour of coroutine in Lib/asyncio/coroutines.py:

def coroutine(func):
    """Decorator to mark coroutines.

    If the coroutine is not yielded from before it is destroyed,
    an error message is logged.
    """
    if inspect.iscoroutinefunction(func):
        # In Python 3.5 that's all we need to do for coroutines
        # defined with "async def".
        return func

    if inspect.isgeneratorfunction(func):
        coro = func
    else:
        @functools.wraps(func)
        def coro(*args, **kw):

this makes the function not being wrapped in CoroWrapper anymore.

@1st1 I don't know if this has a bit more impact than expected. Should I fix this by adding an extra check for partial in coroutine() (check commit 617688e to see what I mean), or should we approach this differently?

…eserve behaviour

Please remove new checks from asyncio.coroutine; AFAICT we don't need them.

We don't need extra checks in this function. Here, we check for an async def coroutine -- we don't need to wrap them in a CoroWrapper ever.

We don't need this extra check either -- if func is a generator function, it means that we don't need to wrap it into a proxy to force it to return a generator.

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@1st1 If I remove those checks then https://github.com/python/cpython/blob/master/Lib/test/test_asyncio/test_tasks.py#L425 will fail because the partials are not wrapped anymore and the __reprl__ will be different. I was not sure if this would be a problem or not (notice commit 617688e was just to show what I mean with "fix this by adding an extra check for partial in coroutine()" in my previous comment). Should I change the tests now that we don't need to wrap these (proposal of that change in commit caddd21)?

This is an example failure of that test for clarity:

======================================================================
FAIL: test_task_repr_partial_corowrapper (test.test_asyncio.test_tasks.PyTask_PyFuture_Tests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/pablogsal/github/cpython/Lib/test/test_asyncio/test_tasks.py", line 446, in test_task_repr_partial_corowrapper
    self.assertRegex(coro_repr, expected)
AssertionError: Regex didn't match: '<CoroWrapper \\w+.test_task_repr_partial_corowrapper\\.<locals>\\.func\\(1\\)\\(\\) running, ' not found in '<coroutine object BaseTaskTests.test_task_repr_partial_corowrapper.<locals>.func at 0x7f30c10b9788>'

----------------------------------------------------------------------

I like the overall change.

@1st1: Would you mind to review this change?

I suggest to move this helper function to Lib/functools.py.

Maybe also document the change with a ".. versionchanged:: 3.8" markup in https://docs.python.org/dev/library/inspect.html#inspect.isgeneratorfunction ?

(Oh, @1st1 already started to review this change, good ;-))

LGTM, but I would prefer to see a second review from a core (@1st1 maybe?).

@1st1 If I remove those checks then https://github.com/python/cpython/blob/master/Lib/test/test_asyncio/test_tasks.py#L425 will fail because the partials are not wrapped anymore and the reprl will be different. I was not sure if this would be a problem or not

It's not a problem. Now that inspect.isgeneratorfunction works with functools.partial we no longer need to wrap partial objects in a proxy generator "just in case".

Sorry for the delayed review; I've been extremely busy last couple of weeks.

@1st1 Thank you very much for the review and the advice, Yuri! :)

See also #6448 for a very related problem.

@pablogsal BTW this doesn't handle the case of partialmethod. Should we add that?

pablogsal self-assigned this Oct 15, 2018

pablogsal requested review from 1st1 and asvetlov October 15, 2018 22:14

the-knights-who-say-ni added the CLA signed label Oct 15, 2018

bedevere-bot added the awaiting merge label Oct 15, 2018

pablogsal changed the title Make iscoroutinefunction, isgeneratorfunction and isasyncgenfunction work with functools.partial bpo-34890: Make iscoroutinefunction, isgeneratorfunction and isasyncgenfunction work with functools.partial Oct 15, 2018

Make iscoroutinefunction, isgeneratorfunction and isasyncgenfunction …

633c497

…work with partial

pablogsal force-pushed the bpo34890 branch from ded4d70 to 633c497 Compare October 15, 2018 22:16

1st1 reviewed Oct 15, 2018

View reviewed changes

pablogsal force-pushed the bpo34890 branch from 3d14641 to da78465 Compare October 15, 2018 22:25

Simplify code, rename variables and add test for nested partials

41f7f60

pablogsal force-pushed the bpo34890 branch from da78465 to 41f7f60 Compare October 15, 2018 22:26

fixup! Simplify code, rename variables and add test for nested partials

b65500e

1st1 reviewed Oct 15, 2018

View reviewed changes

Add _unwrap_partial helper to reduce repetition

3b841ac

1st1 reviewed Oct 15, 2018

View reviewed changes

Use module import in tests

adb5db6

1st1 approved these changes Oct 15, 2018

View reviewed changes

Add extra check to Lib/asyncio/coroutines:coroutine for partial to pr…

617688e

…eserve behaviour

1st1 requested changes Oct 16, 2018

View reviewed changes

bedevere-bot removed the awaiting merge label Oct 16, 2018

bedevere-bot added the awaiting changes label Oct 16, 2018

Change test that checks __reprl__ of partial

caddd21

vstinner reviewed Oct 25, 2018

View reviewed changes

Refactor helper function and document changes

0585757

pablogsal requested a review from rhettinger as a code owner October 25, 2018 22:29

vstinner approved these changes Oct 25, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Oct 25, 2018

1st1 approved these changes Oct 26, 2018

View reviewed changes

pablogsal merged commit 7cd2543 into python:master Oct 26, 2018

bedevere-bot removed the awaiting merge label Oct 26, 2018

pablogsal deleted the bpo34890 branch October 26, 2018 11:19

1st1 reviewed Mar 29, 2019

View reviewed changes

                               object.__code__.co_flags & CO_GENERATOR)
+                  is_partial_generator = bool((isinstance(object, functools.partial) and
+                              object.func.__code__.co_flags & CO_GENERATOR))
+                  return is_generator_function or is_partial_generator

                   """
-                  return bool((isfunction(object) or ismethod(object)) and
-                              object.__code__.co_flags & CO_ASYNC_GENERATOR)
+                  while isinstance(obj, functools.partial):

               import collections
               import datetime
               import functools
+              from functools import partial

+              def _unwrap_partial(func):
+                  while isinstance(func, functools.partial):
+                      func = func.func
+                  return func

@@ @@ -0,0 +1,3 @@ @@
+              Make :func:`inspect.iscoroutinefunction`,
+              :func:`inspect.isgeneratorfunction` and :func:`inspect.isasyncgenfunction`
+              work with :func:`functools.partial`. Patch by Pablo Galindo.

                   """
-                  if inspect.iscoroutinefunction(func):
+                  if (inspect.iscoroutinefunction(func)
+                          and not isinstance(func, functools.partial)):

-                  if inspect.isgeneratorfunction(func):
+                  if (inspect.isgeneratorfunction(func)
+                          and not isinstance (func, functools.partial)):

+              # Helper functions
+              def _unwrap_partial(func):
+                  while isinstance(func, partial):

bpo-34890: Make iscoroutinefunction, isgeneratorfunction and isasyncgenfunction work with functools.partial by pablogsal · Pull Request #9903 · python/cpython · GitHub

Repository navigation

bpo-34890: Make iscoroutinefunction, isgeneratorfunction and isasyncgenfunction work with functools.partial - #9903

bpo-34890: Make iscoroutinefunction, isgeneratorfunction and isasyncgenfunction work with functools.partial#9903pablogsal merged 8 commits intopython:masterpython/cpython:masterfrom pablogsal:bpo34890pablogsal/cpython:bpo34890Copy head branch name to clipboard

Conversation

pablogsal commented Oct 15, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1st1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Oct 16, 2018

Uh oh!

pablogsal commented Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Oct 25, 2018

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

1st1 commented Oct 26, 2018

Uh oh!

1st1 left a comment

Choose a reason for hiding this comment

Uh oh!

pablogsal commented Oct 26, 2018

Uh oh!

jdemeyer commented Nov 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bpo-34890: Make iscoroutinefunction, isgeneratorfunction and isasyncgenfunction work with functools.partial#9903
pablogsal merged 8 commits into
python:masterfrom
pablogsal:bpo34890

pablogsal commented Oct 15, 2018 •
edited by bedevere-bot
Loading

pablogsal commented Oct 15, 2018 •
edited
Loading

pablogsal commented Oct 15, 2018 •
edited
Loading

pablogsal commented Oct 16, 2018 •
edited
Loading

jdemeyer commented Nov 5, 2018 •
edited
Loading