-
-
Notifications
You must be signed in to change notification settings - Fork 954
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
Add support for ASGI pathsend
extension
#2671
base: master
Are you sure you want to change the base?
Conversation
2bbda0a
to
ddfc5c0
Compare
9286c83
to
4631efb
Compare
fe45c1b
to
6b658e8
Compare
@Kludex I just rebased from current master for your convenience |
@Kludex what's your current take on this? Should I rebase so it can be merged, or should we just forget about it? |
I want this to get in, I want to get back to this soon. I can rebase myself. Sorry the delay here. |
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.
This has been going for long - Sorry about it.
The only blocker here is deciding what to do about the changes on the BaseHTTPMiddleware
- but I guess that's really the point of this PR. Would it make sense to remove the extension from the scope?
starlette/middleware/base.py
Outdated
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.
Is altering the scope["extensions"]
in the application side allowed? Would it be bad to remove the http.response.httpsend
from the BaseHTTPMiddleware
? It's a question. I'm not sure what would be best.
I would like to avoid adding complexity to the BaseHTTPMiddleware
.
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 not sure how altering the scope would help in avoiding changes on BaseHTTPMiddleware
.
To my perspective, the only way to avoid these lines https://github.com/encode/starlette/pull/2671/files#diff-931c53548e8908544bd0818ee1a8d5f1ada4f07430a439e9322ab369eb40a8acR161-R165 is to re-implement the whole streaming wrapper, which doesn't sound less complex to me tbh..
Maybe we could try to avoid the response re-wrap in _StreamingResponse
depending on the context? Probably I don't have the needed to knowledge of Starlette internals to propose such a change though :(
9c023f0
to
8d68730
Compare
8d68730
to
d1dbb45
Compare
@Kludex not sure why the coverage fails after the last rebase.. |
Because we recently set |
Summary
As per title, this adds support for ASGI pathsend extension on
FileResponse
class for servers implementing it.This is a re-work of #2435 combined with parts of #2616, as discussed in #2613 with @Kludex.
Checklist