-
-
Notifications
You must be signed in to change notification settings - Fork 800
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
Use the async session.asave method if it exists #2092
Conversation
6b33357
to
0bcd7a4
Compare
0bcd7a4
to
545b4e1
Compare
@carltongibson: I've added the missing test coverage (such as I could, see inline comment) and fixed the thing you noted. I'm not sure this should be merged before #2090 is resolved since sessions can use the Django ORM under the hood. |
545b4e1
to
2ac042f
Compare
Yes, I too was thinking we need to settle that first 👍 |
2ac042f
to
6cfca1e
Compare
Ref the breaking change here: I think if we're super clear in the release notes that may have to do. |
6cfca1e
to
df56f74
Compare
Alright, we've got full test coverage now! |
Once this PR is merged I'll add the backwards compat notices to: #2111 |
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.
OK, bar the one comment (which. I think is messaging rather than a blocker) This looks good. 👍
Support was added in django/django#17372 (slated for release in 5.1)
We can use this API if it exists, otherwise, we fallback to a
sync_to_async
bridge.This does break backwards compatibility, which I'm not terribly happy about but I'm not sure how best to fix it.
If users overrode
save_session
in a subclass they would expect that to still work, so I thought the best way to do this was to make it async in the base class and thenawait
it, so at least it will be obvious that it doesn't work (python will raise an exception aboutawait
ing a non-awaitable), but I'm open to other solutions.