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

Use http::Response to describe response type #161

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tottoto
Copy link
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
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
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