-
-
Notifications
You must be signed in to change notification settings - Fork 838
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
Drop ContentStream #1295
Drop ContentStream #1295
Conversation
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.
Went and skimmed through the changes, and nothing super controversial or popping out of place! Great one. Looking forward to seeing what this allows us to do next :) I really like how requests and responses now behave more and more like pure model classes, with support for both high level APIs and lower level transport arguments and attributes...
🤗 Fantastic, thanks! Let's crack on then. |
Drop
ContentStream
completely. Closes #1253This is a horrendously grungy bit of refactoring to have had to work through, but it's really important.
We no longer have
ContentStream
anywhere internally. Instead any stream interfaces are always typed as plainUnion[Iterable[bytes], AsyncIterable[bytes]]
.request.stream
is no longer aContentStream
instance. Now it isUnion[Iterable[bytes], AsyncIterable[bytes]]
.response._raw_stream
is no longer aContentStream
instance. Now it's promoted to a publicresponse.stream
interface and isUnion[Iterable[bytes], AsyncIterable[bytes]]
.This PR is not a lot of fun at all, but it does get the public API where it needs to be, and it also preps. the ground for us switching up the httpcore API to also not need a
SyncByteStream
/AsyncByteStream
abstraction.We can instead move to using
yield
for the closing management (encode/httpcore#145), and plainIterable[bytes]
/AsyncIterable[bytes]
annotations for the byte streams.It's also worth looking at what the
MockTransport
implementation looks like after this PR, since it helps demonstrate how it helps clean things up...This PR also sorts out some fiddly set wrt. when to automatically include headers on requests or responses.
For example
Response(content=...)
was not currently setting any automatically populatedContent-Length
header, although it really should. We weren't doing so because we don't want responses that are instantiated from the returned info from the Transport API to have auto-headers generated. This PR sorts that out withstream=...
being used for passing raw streams, and not implying any auto-headers, in contrast toResponse(content=...)
which is the more user-facing API for creating responses for mock tests or for other server-side contexts. This unblocks #1250.I don't expect this PR to be actionable reviewable, but we've got our type checking in place all the way through, plus 100% coverage. I'm not expecting any issues, but if there are any unexpected niggles we'd just have to deal with them.