http
http copied to clipboard
hyperium
Replace error-returning builders with typestate-based builders
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)
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.
@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?
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.
Ok. I have added some thoughts. Maybe I should add some more motivation / explanation on the linked PR.
@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 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?
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! :-)
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.
