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.

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

Add a Request type #85

Closed
theduke opened this issue May 8, 2017 · 11 comments
Closed

Add a Request type #85

theduke opened this issue May 8, 2017 · 11 comments

Comments

@theduke
Copy link
Contributor

theduke commented May 8, 2017

When building a client API based on reqwest, I would often want to modify or inspect the private fields on RequestBuilder, namely method, url, headers and body.

Since they are private, you basically have to create your own builder and convert it, which is annoying.

What do you think about either making the fields public or providing getters/setters?

@seanmonstar
Copy link
Owner

Getters would be a good idea here.

@theduke
Copy link
Contributor Author

theduke commented May 8, 2017

@seanmonstar I'm happy to submit PRs for the three issues I opened, just wanted to get some input on them from you first.

theduke added a commit to theduke/reqwest that referenced this issue May 8, 2017
@seanmonstar seanmonstar changed the title Expose RequestBuilder fields Add a Request type May 31, 2017
@seanmonstar
Copy link
Owner

Ok, so we discussed the builder pattern in the libs blitz, and I think I have a clear picture on how this should probably all look.

The builder should be updated according to #108. That struct should look something like this:

pub struct RequestBuilder {
    client: ClientRef,
    request: Option<Request>,
}

All of the builder methods will essentially be doing something conversions (like parsing into a Url, or serializing into JSON, etc), and then calling the internal request.set_url(url) or request.set_body(body), etc.

That means the Request type would have getters and setters, but they wouldn't do any conversions when setting, so they are infallible. For example:

pub struct Request {
    url: Url,
    method: Method,
    body: Option<Body>,
    // ...
}

impl Request {
    pub fn url(&self) -> &Url {
        &self.url
    }

    pub fn set_url(&mut self, url: Url) {
        self.url = url;
    }
}

Lastly, the builder would have 2 methods, build to take the Request out, and send (or execute), to make use of its internal ClientRef. If you have a Request, you can inspect it and continue to manipulate it, and then client.execute(req) at a later time.

Whatcha think of this proposal?

@theduke
Copy link
Contributor Author

theduke commented May 31, 2017

That is pretty much what my PR does (except the builder refactoring).

I just made the fields on Request public and left out getters and setters because they seem redundant to me.

If you assign a Url or a Some(Body) to a request, they already have to be valid.

The only advantage would be the ability to restructure the underlying representation without a breaking change, which might valuable.

@seanmonstar
Copy link
Owner

Yes, we would rather have getter and setter methods, because it allows us to add fields to the struct without breaking matchs, and change around the exact layout.

@theduke
Copy link
Contributor Author

theduke commented May 31, 2017

I'll update the PR.

I can also implement the builder changes, but might not have time until the weekend.

@seanmonstar
Copy link
Owner

I'd consider the builder changes a separate thing, so as to not block the Request PR.

@theduke
Copy link
Contributor Author

theduke commented May 31, 2017

Mhm, the API Guidlines don't offer an opinion regarding naming of getters.

Should it be method(&self) -> &Method or get_method(...)?

I personally prefer to drop the get.

@theduke
Copy link
Contributor Author

theduke commented May 31, 2017

Also, this design does not account for the point I raised regarding extracting values from a request without cloning.

For example, consider I want to add an additional header. Then I'd have to do get the headers, clone them, add my header and then set, which sucks.

So... headers_mut(&mut self) -> &mut Headers?

And maybe a take_body(&mut self) -> Option<Body>?

@seanmonstar
Copy link
Owner

Yes, getters don't tend to have the get_ prefix.

Also, I'd expect a headers_mut() -> &mut Headers instead of a set_headers setter, in this case.

Is there need to mutably get the body?

@seanmonstar
Copy link
Owner

This was done with #103.

repi pushed a commit to EmbarkStudios/reqwest that referenced this issue Dec 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants