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

Allow usage of functools.partial async handlers #984

Merged
merged 10 commits into from Nov 8, 2020

Conversation

vladmunteanu
Copy link
Contributor

At the moment, one cannot pass a partial asynchronous handler, because:

  1. they will be recognized as ASGI and treated as such.
  2. asyncio.iscoroutinefunction fails to detect a partial in Python versions < 3.8 Stackoverflow discussion

@erewok
Copy link
Contributor

erewok commented Jul 1, 2020

This is interesting. I would like to be able to use partial functions as handlers as well and didn't know about this bug. Two questions:

  • In the stackoverflow question you posted, the answer there descends into the partial function using a while loop (included below)1. It states that this is the solution added to Python 3.8. Do we also need to be able to handle more than one layer of partial-wrapping?

  • Since this bug affects Python versions less than 3.8, should we check the version and only run this check for earlier versions (with the idea that Starlette could eventually stop supporting earlier versions after enough time has passed)?

  1. Example function from the Stackoverflow answer:
def iscoroutinefunction_or_partial(object):
    while isinstance(object, functools.partial):
        object = object.func
    return inspect.iscoroutinefunction(object)

@vladmunteanu
Copy link
Contributor Author

@erewok I think my eyes skipped right over that while statement when I initially read the issue.
I think we should definitely check for multiple levels of wrapping too, and I agree we can add a check for py versions and only process this on py < 3.8.

If this sounds good, I'd like to add the method you pointed out (iscoroutinefunction_or_partial), with an extra check for the py versions.

@erewok
Copy link
Contributor

erewok commented Jul 1, 2020

I like those ideas. I would defer to @tomchristie on merging the result.

@vladmunteanu
Copy link
Contributor Author

@erewok I was afraid of this... with the py 3.8 check, we now fall under 100% coverage on that interpreter. Any suggestions? Or should we just dump the check and let the while loop run?

@erewok
Copy link
Contributor

erewok commented Jul 1, 2020

Let's see what @tomchristie thinks about this PR. I'm not sure how he'll want to handle different behavior in different Python versions and don't want to suggest you do a bunch of work. I think the idea is solid overall.

@vladmunteanu
Copy link
Contributor Author

BTW, this is the kind of usage i'm thinking about:
https://github.com/vladmunteanu/starlette-jsonapi/blob/a35dbb6ae5f15d4ce9ccdaad01751445d743f32b/starlette_jsonapi/resource.py#L173

I had to write some methods (ie: handle_get, handle_post) that otherwise might not be needed if partials would be possible.

@kevinastone
Copy link
Contributor

@vladmunteanu You can add a # pragma: no cover to the python version check line to ignore that workaround for code coverage purposes.

Example in my recent PR:
https://github.com/encode/starlette/pull/1007/files#diff-f43e94ab436a8cbb273305cbe72b36e3R38

@vladmunteanu
Copy link
Contributor Author

vladmunteanu commented Jul 24, 2020

@kevinastone thanks, I was going to propose that, but I was also curious what @tomchristie has to say about this before. I guess I should just do it to increase my chances.

@vladmunteanu
Copy link
Contributor Author

Hey @erewok , thanks for the approval!
Any idea when it will be released?

@erewok
Copy link
Contributor

erewok commented Oct 31, 2020

hello @vladmunteanu, sorry for the late reply, but I haven't been available for open source for months due to work obligations. I will check with the encode maintainers this week to see if everyone is okay with releasing this. Thanks again.

Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

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

Just checked out this PR and this looks good to me. :)

@erewok erewok merged commit fe961dd into encode:master Nov 8, 2020
@vladmunteanu
Copy link
Contributor Author

Thanks everyone for getting this through!

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.

None yet

4 participants