Implements Response.from_response and support for cf opts in Py fetch. #3229
Conversation
| ): | ||
| options = { | ||
| "status": status.value if isinstance(status, HTTPStatus) else status, | ||
| } | ||
| if len(statusText) > 0: | ||
| options["statusText"] = statusText | ||
| if len(status_text) > 0: |
There was a problem hiding this comment.
This does the same thing afaict:
| if len(status_text) > 0: | |
| if status_text: |
There was a problem hiding this comment.
Explicit is always better than implicit
There was a problem hiding this comment.
Well it's most common in python code to check if a container is non empty with if container:. I think you'll find this suggested in most style guides as the preferred way.
| @@ -117,15 +139,25 @@ def __init__( | |||
| ) | |||
| super().__init__(js_resp.url, js_resp) | |||
|
|
|||
| @classmethod | |||
| def from_response(cls, body: Body, response: "Response") -> "Response": | |||
There was a problem hiding this comment.
from_response is a bit of an odd name. Is there an existing api like this? I would think to make it a method called replace_body. e.g,
def replace_body(self, new_body: Body) -> "Response":
"""
Returns a new Response object with the same headers but an updated body
""""
| @@ -99,7 +121,7 @@ def __init__( | |||
| self, | |||
| body: Body, | |||
| status: HTTPStatus | int = HTTPStatus.OK, | |||
| statusText="", | |||
| status_text="", | |||
There was a problem hiding this comment.
Formatting is inconsistent here, does our lint check not check formatting?
| result = cls.__new__(cls) | ||
| js_resp = js.Response.new(b, response.js_object) | ||
| super(Response, result).__init__(js_resp.url, js_resp) |
There was a problem hiding this comment.
Can you explain the reasoning for this? Ideally we should switch to:
| result = cls.__new__(cls) | |
| js_resp = js.Response.new(b, response.js_object) | |
| super(Response, result).__init__(js_resp.url, js_resp) | |
| js_resp = js.Response.new(b, response.js_object) | |
| return cls(js_resp.url, js_resp) |
or type(self)(js_resp.url, js_resp) assuming we make it into an instance method. If this is different in some important way we'll need a comment. Particularly, I would expect super(cls) instead of super(Response) which skips the __init__ functions of any subclasses.
| # * that other options can be passed into `fetch` (so that we can support | ||
| # new options without updating this code) | ||
| resp = await fetch( | ||
| "https://example.com/redirect", redirect="manual", foobarbaz=42 | ||
| ) |
There was a problem hiding this comment.
I guess we could test that we receive foobarbaz on the other side by mocking the JS fetch. In this case, we just test that we don't crash with this option but we could discard it without failing the test?
While going through our examples I noticed that these were missing.