-
-
Notifications
You must be signed in to change notification settings - Fork 857
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
Switch to request context manager interface #1355
Conversation
Fantastic! So, things to note here... at some point I mentioned that we wouldn't need Here's the deal... with httpx.stream("GET", "https://www.example.com") as response:
pass
response.read() Currently that'll raise We could just drop So, I think we do end up wanting to keep
?... |
@tomchristie Yup, I got to the point of just removing |
The main thing I don't like about this anymore is the breaking change " |
Alternative to #1341, taking into account the POC here: https://gist.github.com/tomchristie/e6e5e06b6c7b77b9c726b74d1ba0acbc
The change footprint in this PR is much larger than #1341, because this also changes the following, introducing breaking API changes:
Response
drops.close()
and.aclose()
, and.is_closed
. In fact, we don't do any tracking of "open/closed" stream state on responses anymore, because we don't need it anymore. We also drop theResponseClosed
exception as a result.Client.send()
changes from returning an "open"Response
to aContextManager[Response]
.AsyncClient.send()
changes from returning an "open"Response
to aAsyncContextManager[Response]
.StreamContextManager
disappears — we don't need it anymore.Docs also updated to reflect these items.
Obviously the scope is larger than what #1341 does, but it's because this PR includes very-likely follow-ups to #1341 that we'd have had to do after #1341 anyway, to keep things most consitent.
Still worth noting that, just like #1341, this PR keeps HTTPX working against HTTPCore 0.12, or HTTPCore once encode/httpcore#206 is merged… (Thanks to the
ensure_(async_)context_manager
helpers.)