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.

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

Merged
merged 12 commits into from
Sep 18, 2020
Merged

Drop ContentStream #1295

merged 12 commits into from
Sep 18, 2020

Conversation

tomchristie
Copy link
Member

@tomchristie tomchristie commented Sep 17, 2020

Drop ContentStream completely. Closes #1253

This 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 plain Union[Iterable[bytes], AsyncIterable[bytes]].

  • request.stream is no longer a ContentStream instance. Now it is Union[Iterable[bytes], AsyncIterable[bytes]].
  • response._raw_stream is no longer a ContentStream instance. Now it's promoted to a public response.stream interface and is Union[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 plain Iterable[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...

class MockTransport(httpcore.SyncHTTPTransport):
    def __init__(self, handler: Callable) -> None:
        self.handler = handler

    def request(
        self,
        method: bytes,
        url: Tuple[bytes, bytes, Optional[int], bytes],
        headers: List[Tuple[bytes, bytes]] = None,
        stream: httpcore.SyncByteStream = None,
        timeout: Mapping[str, Optional[float]] = None,
    ) -> Tuple[bytes, int, bytes, List[Tuple[bytes, bytes]], httpcore.SyncByteStream]:
        request = httpx.Request(
            method=method,
            url=url,
            headers=headers,
            stream=stream,
        )
        request.read()
        response = self.handler(request)
        return (
            (response.http_version or "HTTP/1.1").encode("ascii"),
            response.status_code,
            response.reason_phrase.encode("ascii"),
            response.headers.raw,
            response.stream,
        )

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 populated Content-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 with stream=... being used for passing raw streams, and not implying any auto-headers, in contrast to Response(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.

@tomchristie tomchristie added the refactor Issues and PRs related to code refactoring label Sep 17, 2020
Copy link
Member

@florimondmanca florimondmanca left a 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...

@tomchristie
Copy link
Member Author

tomchristie commented Sep 18, 2020

🤗 Fantastic, thanks! Let's crack on then.

@tomchristie tomchristie merged commit fbb21fb into master Sep 18, 2020
@tomchristie tomchristie deleted the drop-contentstream branch September 18, 2020 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Issues and PRs related to code refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exposing the content stream API
2 participants