Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Async implementation #8617
Async implementation #8617
Changes from 9 commits
79ce07b
67ebe90
7d2b73a
8fa1b7c
daac20a
0a40010
ae84d97
351ea7d
0400834
5e3140a
472ca5b
7b18380
053eabf
f65e859
f16767f
c6cf2d7
ec1d754
de6df46
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A type checker would not be happy about
async_to_sync = None
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
async_to_sync
is available in Django since version 3.1. In this case I'm using it just to run the test. If pytest-asyncio is added then it can be removed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would personally rather we take a testing dependency on
pytest-asyncio
rather than implement a test-only compat helper.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's best to defer the decision to add an additional dependency to project leads. Especially if the new dependency can be easily circumvented, such as in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an a user of drf I would not happy to have any async tests related dependency in my project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to keep this function named
dispatch
to prevent people's overrides from breaking.Which means the only reasonable name for the other function is probably
adispatch
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scratch that, overriding
dispatch
would still work for most people with the current implementation, but would definitely lead to some unexpected behavior if they're trying to upgrade to async.Don't think there's anything we can do about that.
However, the comment above
sync_dispatch
may need to be updated now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this pytest ext be added?
https://pypi.org/project/pytest-asyncio/
I'd expect more async code be added to this repo in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll be happy to add it if there is an agreement instead of using
async_to_sync
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm personally indifferent. I tend to avoid additional dependencies when possible, so I personally support using
asgiref.async_to_sync
instead ofpytest-asyncio
.I don't expect this repo to have much more in terms of asyncio related tests beyond this, since we're already covering Django's CBV and FBV.
Unless Tom Christie feels otherwise, I'd say it's safe to stay as-is.