Change RequestBuilder methods to own a builder

[KamilaBorowska]

This pull request is for 0.9.x branch, not master.

Warning: This is a breaking change, but this isn't 1.0.0 and the changes that will be required due to this change will likely be minimal for most usages (if there will be changes needed to begin with).

This means that build cannot possibly panic anymore due to being called multiple times. This is a breaking change as it breaks the behaviour of builder methods called without assigning to a new variable or chaining. It's rather easy to fix those usages, as they won't compile anymore and can be fixed by assigning a result.

Additionally, this change reduces the size of RequestBuilder, although this likely isn't all that meaningful, as usually there is no reason to store builders in structures.

[seanmonstar]

Thanks for looking into this!

It turns out that the &mut builder pattern was indeed on purpose (the change). It used to be a by-value builder, but the argument was made that a &mut builder is in the end, more flexible.

See also the relevant recommended API guideline.

[KamilaBorowska]

I disagree with this guideline, the only point where I possibly agree is that "Reassignment is annoying", but even then, eh, it's not that annoying, especially when request has plural methods like headers which pretty much avoid the need to reassign.

The other point, however "Hard to give back ownership of the builder after error" doesn't really apply to reqwest - the API isn't really designed to be used with invalid arguments, if you want to handle specific errors anyway, you can use Request API instead of RequestBuilder. Not that you may want to do that, as the only real error you would want to catch is passing non-URL to request builder - the rest is type errors that Rust cannot really catch due to serialization limitations.

Also, interestingly in #108 the ticket author says that "Also they should fail fast where applicable", but this is no longer the case in the API. A builder only fails when trying to convert it into a request, not earlier.

In meanwhile, ownership has its advantages.

It's easy to make a function providing a partial builder.

This is what it looks like right now. Note that this cannot be chained, as &mut ClientBuilder cannot be returned as this wouldn't pass the ownership.

fn request_for_url(client: &Client, url: &str) -> RequestBuilder {
    let mut builder = client.post(&format!("http://example.com/{}", url));
    builder.header(UserAgent::new("foo"));
    builder
}

And this is how it could look like.

fn request_for_url(client: &Client, url: &str) -> RequestBuilder {
    client.post(&format!("http://example.com/{}", url))
        .header(UserAgent::new("foo"))
}

Trying to reuse a builder ends in an compile-time error

The type system enforces not reusing a builder.

An owning builder can be easily specified as must_use

While it's impractical to do that with mutating builders as it causes false error messages, it's not really an issue for owning builders as unused builder is clearly a bug.

[seanmonstar]

FWIW, I also lean more towards having a by-value builder, but there was some strong pressure to have a by-ref builder instead.

However, part of the original argument to switch was to allow getting the builder back if any of the build steps errored, but the builder now just holds the error until the end, so that argument is likely no longer valid...

[seanmonstar]

I'm re-considering the application of the guideline in this crate. I've never liked it, but reading the old reasonings, I feel they don't even apply anymore.

[seanmonstar]

I've rebased this into master, thanks!