Add support for ASGI pathsend extension#2671
Conversation
gi0baro
force-pushed
the
asgi-pathsend-rework
branch
2 times, most recently
from
2bbda0a to
ddfc5c0
Compare
gi0baro
force-pushed
the
asgi-pathsend-rework
branch
2 times, most recently
from
9286c83 to
4631efb
Compare
gi0baro
force-pushed
the
asgi-pathsend-rework
branch
2 times, most recently
from
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. |
Kludex
left a comment
Kludex
left a comment
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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 :(
gi0baro
force-pushed
the
asgi-pathsend-rework
branch
3 times, most recently
from
8d68730 to
d1dbb45
Compare
|
@Kludex not sure why the coverage fails after the last rebase.. |
Because we recently set |
gi0baro
force-pushed
the
asgi-pathsend-rework
branch
from
d1dbb45 to
6d23fba
Compare
|
@Kludex sorry for the long delay. This is now rebased on latest master, and tests/CI is finally green. Let me know if any additional edits are required for merge. |
|
Sorry the delay. Gradly you caught me at PyCon IT. 😅 |
3 participants
Summary
As per title, this adds support for ASGI pathsend extension on
FileResponseclass for servers implementing it.This is a re-work of #2435 combined with parts of #2616, as discussed in #2613 with @Kludex.
Checklist