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

Feature Request: ResponseBuilder #333

Closed
pwoolcoc opened this issue Aug 25, 2018 · 4 comments
Closed

Feature Request: ResponseBuilder #333

pwoolcoc opened this issue Aug 25, 2018 · 4 comments
Labels
B-rfc Blocked: Request for comments. More discussion would help move this along.

Comments

@pwoolcoc
Copy link
Contributor

Related to this comment: #154 (comment)

It would be nice to be able to manually build Response objects so we can mock out the HTTP interactions in tests
@seanmonstar
Copy link
Owner

Hm, perhaps adding a From<http::Response> for reqwest::Response, or something like that?

@seanmonstar seanmonstar added the B-rfc Blocked: Request for comments. More discussion would help move this along. label Sep 19, 2018
@pwoolcoc
Copy link
Contributor Author

Yea, that would be perfect, actually

@pwoolcoc
Copy link
Contributor Author

pwoolcoc commented Oct 4, 2018

I've been trying to write this, and have run up against a couple of issues. First, I planned on actually writing two From<http::Response> implementations, one for async_impl::Response and one for Response. The impl for async_impl::Response is fairly straightforward, except for the fact that async_impl::Response expects a Box<Url> with the final URL. http::Response doesn't have this data (or at least, doesn't expose it through it's API), so right now I'm using the .extensions() mechanism to retrieve a Url from the http::Response. However, that returns an Option so I'm not sure what to do if there is no Url. Some sentinel value perhaps?

The other issue is that Response carries a reference to the InnerClientHandle through the KeepCoreThreadAlive type, and I have no idea how to get that from just a From<http::Response> impl.

Do you have any thoughts on this @seanmonstar?

@seanmonstar
Copy link
Owner

Yea, I noticed those too when suggesting it...

  • Looking in extensions for a Url is also what I was thinking... I'd suggest it should be a unique wrapper newtype, since Url by itself could have different meanings if extensions were filled by other libraries. If it doesn't exist, a sentinel value could make sense...
  • The KeepCoreThreadAlive type could be changed to hold an Option<Arc<InnerClientHandle>>, and then an empty one can be created.

@pwoolcoc pwoolcoc mentioned this issue Oct 4, 2018
Merged
seanmonstar pushed a commit that referenced this issue Oct 5, 2018
@seanmonstar
From<http::Response> for Response (#360)
4857a59 
This adds an implementation to convert a `Response` type from the `http`
crate to the `async_impl::Response` type. This is the first step to
allow us to convert `http::Response` objects to `request::Response`
objects

This also adds an extension trait for the `http::response::Builder`
type. The `http::Response` object does not provide a way to access the
"final" url that the response is derived from, so we can't easily
provide that in the `From<http::Response>` implementation. For users who
are manually constructing `http::Response` objects for use in tests,
etc, they can import this extension trait, which adds a `.url()` builder
method that will allow them to pass a `Url`, which we then convert to
our newtype'd Url and add to the `http::Response`'s `extensions`. Then,
when converting from `http::Response` to `async_impl::Response` we can
pull that value out of the `extensions` and use it to construct the
`async_impl::Response`

Closes #333
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
B-rfc Blocked: Request for comments. More discussion would help move this along.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@seanmonstar @pwoolcoc