Response builder and related changes #580

jbr · 2020-06-10T19:32:48Z

This PR does several things that are related, but could be distinct PRs. Please let me know if you'd prefer I break them out for independent review.

changes insert_ext from taking mut self to taking &mut self to conform to the rest of the response interface
allows Response::set_status to take any TryInto<StatusCode> like Response::new() does

let mut res = Response::new(StatusCode::Ok);
res.set_status(203);

~~provides a From<Body> for Response and uses that conversion in other Body -> Response conversions that already existed.~~ Extracted to provide a From<Body> for Response #584
adds a response builder:

app.at("/").get(|_| async {
    Ok(Response::builder(203)
        .body("body")
        .header("custom-header", "value"))
})

Fishrock123

I don't have much critique on builders in general, ~~though I do think if we do this we should also do RequestBuilder, for api conformity.~~

The code certainly seems fine to me, it's nice that ResponseBuilder is essentially a tuple and probably zero-ish-cost.

Re: other changes - I think From<Body> for Response is probably useful enough to already take out and merge. I literally already thought that existed!

src/response_builder.rs

Fishrock123 · 2020-06-10T21:30:49Z

src/response_builder.rs

+
+pub struct ResponseBuilder(Response);
+
+impl ResponseBuilder {


Should this include methods for cookie and ext?

I initially added those, but I don't think they make a lot of sense on a builder. The purpose of the builder is to make expressing a one-off response easy. I think it's pretty reasonable to say if they need to make cookie or ext changes, that it's not a quick one-liner type response (before rustfmt). It's easy enough to do:

let mut res = Response::build().status(203).body("hello").unwrap(); res.add_cookie(…); res.insert_ext(…);

Not exactly seeing the advantage of that to writing this if it is only partial?
The same example with less characters:

let mut res = Response::new(203); res.set_body("hello"); res.add_cookie(…); res.insert_ext(…);

Agreed, in that circumstance there isn't any advantage to using the builder

jbr · 2020-06-10T22:11:47Z

@Fishrock123 I agree surf might want a RequestBuilder, but why would tide? App authors aren't ever constructing requests

Fishrock123 · 2020-06-11T01:24:10Z

Good point, I think for some reason I was thinking this was in http-types.

yoshuawuyts · 2020-06-12T14:57:19Z

Thanks so much for kicking this off @jbr! I'm not quite convinced on builders yet, but think this is def worth exploring. With a bit of luck this indeed turns out to be a much more convenient way to create Responses and that would be neat. I do have a few design notes though:

We should rename the main method to Response::builder; build is the conventional name for the finalizer method, builder for the constructor.
Response::builder should take Into<StatusCode> as the argument. It's 3 extra characters, that in many cases will save an extra line. It also creates parity with Response::new which doesn't assume a default status code either. For surf we'd require any form of Request::builder to take a url as an argument, so this creates symmetry there as well.
Many of the examples use .into() as the finalizer. We should add build as an explicit method for this as well.
We can remove the need to invoke the finalizer method in most cases if we change the return type of the enclosing function from -> tide::Result into -> tide::Result<Into<Response>>. This allows ResponseBuilder::Into to be called behind the scenes and saves an extra line in many cases.
4b. Note that we can't yet express tide::Result as http_types::Result<impl Into<tide::Response>>. This requires lang support.
In the examples for ResponseBuilder we should show the path we want people to take: namely by importing tide::Response and calling Response::builder. That's the canonical way to gain access to the struct, and I think we should use that in our examples as well.

jbr · 2020-06-12T21:54:20Z

Addressed 1, 2, 3, and 5. Added one example of 4, but not convinced it's particularly convenient to type in most circumstances

src/response_builder.rs

tests/chunked-encode-small.rs

tests/chunked-encode-large.rs

examples/chunked.rs

Co-authored-by: Jeremiah Senkpiel <fishrock123@rocketmail.com>

yoshuawuyts

💯

jbr requested review from yoshuawuyts and Fishrock123 June 10, 2020 19:32

jbr force-pushed the response-builder branch 2 times, most recently from 74dc6f3 to 3f06463 Compare June 10, 2020 21:01

Fishrock123 reviewed Jun 10, 2020

View reviewed changes

jbr mentioned this pull request Jun 10, 2020

provide a From<Body> for Response #584

Merged

jbr force-pushed the response-builder branch from 4a266e6 to 02338d3 Compare June 11, 2020 21:55

jbr force-pushed the response-builder branch from 02338d3 to d6c293c Compare June 12, 2020 21:53

jbr force-pushed the response-builder branch from e3410b0 to c9463fe Compare June 13, 2020 02:34

jbr mentioned this pull request Jun 14, 2020

Proposal: Return Into<Response> from endpoints #596

Closed

jbr added the enhancement New feature or request label Jun 19, 2020

yoshuawuyts reviewed Jun 21, 2020

View reviewed changes

src/response_builder.rs Outdated Show resolved Hide resolved

tests/chunked-encode-small.rs Show resolved Hide resolved

tests/chunked-encode-large.rs Show resolved Hide resolved

examples/chunked.rs Show resolved Hide resolved

jbr and others added 9 commits June 21, 2020 14:56

insert_ext should not take ownership of self

04e8640

allow setting a response status from any TryInto<StatusCode>

09bef88

add response builder

36c6fac

update examples and tests

748f0f4

add docs for ResponseBuilder

bc59e7f

fix mistake in ResponseBuilder::content_type docs

eb89ef8

Co-authored-by: Jeremiah Senkpiel <fishrock123@rocketmail.com>

Response::builder(status) instead of Response::build()

d4cb305

add an example of returning Ok(ResponseBuilder)

3de967e

remove Into<Result>, use ok-wrapping

43dfdc3

jbr force-pushed the response-builder branch from c9463fe to 43dfdc3 Compare June 21, 2020 22:03

yoshuawuyts approved these changes Jun 27, 2020

View reviewed changes

jbr merged commit f03ca44 into http-rs:master Jun 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Response builder and related changes #580

Response builder and related changes #580

jbr commented Jun 10, 2020 •

edited

Loading

Fishrock123 left a comment •

edited

Loading

Fishrock123 Jun 10, 2020

jbr Jun 10, 2020

Fishrock123 Jun 10, 2020

jbr Jun 10, 2020

jbr commented Jun 10, 2020

Fishrock123 commented Jun 11, 2020

yoshuawuyts commented Jun 12, 2020

jbr commented Jun 12, 2020 •

edited

Loading

yoshuawuyts left a comment

Response builder and related changes #580

Response builder and related changes #580

Conversation

jbr commented Jun 10, 2020 • edited Loading

Fishrock123 left a comment • edited Loading

Choose a reason for hiding this comment

Fishrock123 Jun 10, 2020

Choose a reason for hiding this comment

jbr Jun 10, 2020

Choose a reason for hiding this comment

Fishrock123 Jun 10, 2020

Choose a reason for hiding this comment

jbr Jun 10, 2020

Choose a reason for hiding this comment

jbr commented Jun 10, 2020

Fishrock123 commented Jun 11, 2020

yoshuawuyts commented Jun 12, 2020

jbr commented Jun 12, 2020 • edited Loading

yoshuawuyts left a comment

Choose a reason for hiding this comment

jbr commented Jun 10, 2020 •

edited

Loading

Fishrock123 left a comment •

edited

Loading

jbr commented Jun 12, 2020 •

edited

Loading