hyperium / http

Rust HTTP types
Apache License 2.0
1.16k stars 291 forks source link

Replace error-returning builders with typestate-based builders #330

Open davidbarsky opened 5 years ago

davidbarsky commented 5 years ago

It'd be really handy to replace the current builders with a set of builders for Request, Response, and URI that track the builder's state using some sort of typestate, allowing for compile-time checks that a Request/Response/URI were correctly built.

(Following up from Twitter: https://twitter.com/seanmonstar/status/1149772057212669952)

kaj commented 4 years ago

Agreed. When looking into why Response::builder().body("Hello") can fail at all, the main reason seems to be that a lot of methods of Builder takes T: TryFrom<A> where A is the actually wanted type, instead of T: Into<A>. This might be convenient at times, but I think they should take Into<A> or even A, and then the caller can decide if a fallible conversion is wanted (and handle the error) or if in infallible conversion or a ready-made actual value should be used instead.

Take the status method for exampe. It might seem nice to be able to write builder.status(304), but that makes it possible to write builder.status(75), which leaves the builder in a broken state. If we instead insisted in builder.status(StatusCode::PERMANENT_REDIRECT) the builder don't need to have a broken state. We could even add a function to enable builder.try_status(75), but that would return a Result<Builder, http::Error>, i.e. either a builder in a good state or no builder at all.

kaj commented 3 years ago

@seanmonstar , how does the B-rfc tag work, is there ongoing discussions somewhere else or is this thread the place for discussion? Is there a thread on users.rust-lang.org? Should I open one?

seanmonstar commented 3 years ago

The tag is meant to say the issue needs more comments about how and if to move forward. The discussion can happen on the issue.

kaj commented 3 years ago

Ok. I have added some thoughts. Maybe I should add some more motivation / explanation on the linked PR.

stappersg commented 3 years ago

@kaj please pick the best from #442 #448 and #454 and close the other two.

In other words: Reduce to amount of open merge request. Please

kaj commented 3 years ago

@kaj please pick the best from #442 #448 and #454 and close the other two.

They fix separate parts of the problem, so I think they all are needed. If one bigger PR is considered better than three smaller I can merge them to one single PR?

stappersg commented 3 years ago

Aim for fewer open merge requests.

kaj commented 3 years ago

Aim for fewer open merge requests.

Ok, #483 replaces the previous PRs.

kaj commented 3 years ago

There's still not much discussion here. Maybe that's because there isn't that much to discuss? Is there any actual downsides to replacing the current run-time checks in the builder patterns with compile-time typestate-based checks? This is a breaking API change (even if #483 is a non-breaking first step), so it should be taken with caution. But this is a breaking API change, so we should really want it to happen as soon as possible, way before releasing a version 1.0 of the crate.

So, for the issue itself, I'm pretty sure there is nothing that should stop this. For the implementation suggested in #483 , there may or may not be problems. Please review! :-)

lkts commented 3 years ago

I would like to see this change. I was surprised to see body on Response returning Result because i didn't see a way to handle it properly. As a result you have to resort to expect essentially removing any value of Result.