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

Fix #679: allow async class methods as dependencies #681

Merged
merged 1 commit into from
Nov 27, 2019

Conversation

frankie567
Copy link
Contributor

This is the fix for issue #679: allow to use async class methods as dependencies.

Before, such methods were not correctly detected as coroutine and thus were not awaited but rather sent to run_in_threadpool. We ended up then injecting the coroutine instead of its result.

I've used inspect.isroutine, instead of doing inspect.isfunction(call) or inspect.ismethod(call). It seems to fulfill our need while saving us a check.

I also took this opportunity to add some unit tests to verify the different use cases regarding class dependencies (callable, async callable, sync methods and async methods).

It could also be interesting to mention those possibilities in the Classes as Dependencies section of the documentation. What do you think?

@frankie567
Copy link
Contributor Author

The CI fails because of linting issues (not related to my changes). Should I fix this or do you prefer a separate PR?

@euri10
Copy link
Contributor

euri10 commented Nov 5, 2019 via email

@frankie567
Copy link
Contributor Author

frankie567 commented Nov 5, 2019

Yeah, they fixed something regarding function parameters/trailing commas. Should be quite straightforward to fix, but maybe it should live in another PR. I'll do that quickly.

EDIT: See #682.

@euri10 euri10 mentioned this pull request Nov 19, 2019
@prostomarkeloff
Copy link
Contributor

prostomarkeloff commented Nov 23, 2019

@tiangolo what's about merging it?

@codecov
Copy link

codecov bot commented Nov 23, 2019

Codecov Report

Merging #681 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #681    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files         271    276     +5     
  Lines        7011   7871   +860     
======================================
+ Hits         7011   7871   +860
Impacted Files Coverage Δ
fastapi/dependencies/utils.py 100% <100%> (ø) ⬆️
tests/test_dependency_class.py 100% <100%> (ø)
...ration_advanced_configurations/test_tutorial003.py 100% <0%> (ø) ⬆️
tests/test_jsonable_encoder.py 100% <0%> (ø) ⬆️
fastapi/openapi/utils.py 100% <0%> (ø) ⬆️
fastapi/dependencies/models.py 100% <0%> (ø) ⬆️
docs/src/response_model/tutorial003.py 100% <0%> (ø) ⬆️
fastapi/concurrency.py 100% <0%> (ø) ⬆️
fastapi/applications.py 100% <0%> (ø) ⬆️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5f5e63...bbd736f. Read the comment docs.

@frankie567
Copy link
Contributor Author

Rebased the branch with fixed linting to have the pipeline checked 👍

@tiangolo tiangolo merged commit f3ddc7b into tiangolo:master Nov 27, 2019
@tiangolo
Copy link
Owner

Great, thank you @frankie567 ! Thanks for implementing it, and adding extended tests. 🤓 🚀 🍰 🎉

And thanks everyone for the discussion.

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