Skip to content

Use http::Response to describe response type#161

Open
tottoto wants to merge 3 commits intosbstp:masterfrom
tottoto:response
Open

Use http::Response to describe response type#161
tottoto wants to merge 3 commits intosbstp:masterfrom
tottoto:response

Conversation

@tottoto
Copy link
Copy Markdown
Contributor

@tottoto tottoto commented Jan 27, 2024

This is like a RFC rather than a proposal. Uses http::Response type to describe a response type instead of defining an own type. When users are familiar with http crate which is one of the famous libraries for HTTP types, users can use it intuitively. And attohttpc can free from maintaining the response type itself by delegating it like header types. The first commit changes to use http::Response and moves the existing APIs to ResponseExt trait which is newly introduced, except for the following two APIs. By this changes, The Write trait implementation for Response is dropped as Rust's trait rule, and the ability to get url from Response is removed as it is not provided by http crate. I think the both of them might be small backward as they have workaround which is easily achieved. The second commit removes APIs which are simply passed to ResponseReader. These APIs are actually for the body, but since they are existing in Response type, it is possible to be misleading as it is for the entire response message.

@tottoto tottoto changed the title Refactor response type Use http::Response to describe response type Jan 27, 2024
@adamreichold
Copy link
Copy Markdown
Contributor

The second commit removes APIs which are simply passed to ResponseReader. These APIs are actually for the body, but since they are existing in Response type, it is possible to be misleading as it is for the entire response message.

Considering typical usage, even within the tests here, I'd say forcing users to add calls to into_body is not worth the conceptual purity of having these methods only on BodyReader.

@tottoto
Copy link
Copy Markdown
Contributor Author

tottoto commented Feb 3, 2024

Thanks for the feedback. That's exactly what I was thinking of as well. Given the philosophies of the easiness to use, it should be better to keep these APIs. Reverted the second commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants