Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Supporting Response(content=..., text=..., html=..., json=...) #1227

Closed
tomchristie opened this issue Aug 27, 2020 · 3 comments · Fixed by #1297
Closed

Supporting Response(content=..., text=..., html=..., json=...) #1227

tomchristie opened this issue Aug 27, 2020 · 3 comments · Fixed by #1297
Labels
enhancement New feature or request
Milestone
v1.0

Comments

@tomchristie
Copy link
Member

We’d like to support instantiating Response instances directly.

One really good example of why we might want to do that is for unit testing custom auth classes - #1217 (comment)

Currently Response instances are only ever created by the client itself, and...

  • Have a mandatory request=... parameter.
  • Have a mandatory status_code parameter.

The first things we should do here are:

  • Make the request parameter become optional. Have a .request property that returns a Request instance or raises a RuntimeError if the instance is not set. Have a property setter for the .request property.
  • Make the status code be enforced as kwarg-only, and make it optional, with a default of 200.

Once we’ve done those things we should add support for setting the content of a response, using content=..., text=..., html=..., json=... parameters, which mirror the request parameters of data=..., files=..., json=..., but differ because HTTP responses use different content types to HTTP requests.

The encode(...) method in content_streams.py would become encode_request_body(data=..., files=..., json=..., boundary=...), and would be mirrored by a encode_response_body(content=..., text=..., html=..., json=...) method, which would use the following content streams:

  • content - Support either bytes or byte iterator/aiterators. Setting Content-Length or Transfer-Encoding: chunked
  • text - Set Content-Type: text/plain; charset=“utf-8” , and a Content-Length header.
  • html - Set Content-Type: text/html; charset=“utf-8” and a Content-Length header.
  • json - Set Content-Type: application/json and a Content-Length header.

We can tie in an encoding=... argument too, but let’s talk that through as a follow up.

What this would then all give us is the ability to create responses like so...

response = httpx.Response(json={“hello”: “world”})
response = httpx.Response(html=“<body><h1>hello, world</h1></body>”})
response = httpx.Response(text=“Not found”, status_code=404)
response = httpx.Response(content=b’...’, headers=...)

Somewhat refs #1091
@tomchristie tomchristie added the enhancement New feature or request label Aug 27, 2020
@tomchristie tomchristie added this to the v1.0 milestone Aug 27, 2020
@tomchristie
Copy link
Member Author

If any contributors want to take a go at this I'd suggest approaching it in parts...

  • Issue a PR for making the request=... parameter not mandatory.
  • Issue a PR making status_code a keyword-only argument, and defaulting it to 200.
  • Issue a PR with the response body encoding handling (more complex than the other two.)

@Pradhvan
Copy link

The issue is quite detailed thanks for that. 😄 I don't have much experience with httpx but would love to give it a try.

@tomchristie
Copy link
Member Author

Great. Definitely treat each of those items in isolation as separate PRs. 😀

@j178 j178 mentioned this issue Aug 28, 2020
Closed
@tbascoul tbascoul mentioned this issue Aug 30, 2020
Merged
@tomchristie tomchristie mentioned this issue Sep 2, 2020
Closed
@tomchristie tomchristie mentioned this issue Sep 18, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
v1.0
2 participants
@tomchristie @Pradhvan