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

Introduce Request type and refactor execution flow. #103

Closed
wants to merge 2 commits into from

Conversation

theduke
Copy link
Contributor

@theduke theduke commented May 27, 2017

This PR introduces a Request type that represents a
ready to execute request.

A Request can be obtained with RequestBuilder::build(), and executed
with Client::execute().

The RequestBuilder now also builds a Request and forwards it to the
inner client.

The execution logic was moved from Requestbuilder::send() to
ClientRef::execute_request().

@theduke theduke changed the title Introducte Request type and refactor flow. WIP: Introducte Request type and refactor flow. May 27, 2017
@theduke theduke changed the title WIP: Introducte Request type and refactor flow. WIP: Introduce Request type and refactor flow. May 27, 2017
@seanmonstar
Copy link
Owner

@theduke I'm actually going to bring this up during the reqwest lib blitz meeting (Tuesday), as the API design is already a point of discussion in there.

@seanmonstar
Copy link
Owner

I commented in #85 about this.

@theduke theduke changed the title WIP: Introduce Request type and refactor flow. Introduce Request type and refactor execution flow. May 31, 2017
@theduke
Copy link
Contributor Author

theduke commented May 31, 2017

This should be pretty much ready now, @seanmonstar.

Let me know if you need any changes.

Copy link
Owner

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks excellent!

Of course, the changes to the builder will mean that build() shouldn't actually error (all errors will be returned while configuring, instead of stored like now), but that's details for a builder PR.

src/client.rs Outdated
}

/// Takes the body, leaving None in it's place.
pub fn take_body(&mut self) -> Option<Body> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is rather useful, then perhaps it and the setter should be combined into a single body_mut(&mut self) -> &mut Option<Body>?

Copy link
Contributor Author

@theduke theduke May 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it's neccessary, but we might as well add a way to do it. Someone will need it eventually.

I wasn't so sure about it, but I went for take_ now because it seemed weird to work with &mut Option<_>.

I'm not invested in either variant, though. I you prefer body_mut, I'll change it quickly.

A Request can be obtained with RequestBuilder::build(), and executed
with Client::execute().

The RequestBuilder now also builds a Request and forwards it to the
inner client.

The execution logic was moved from Requestbuilder::send() to
ClientRef::execute_request().
@seanmonstar
Copy link
Owner

Merged via e9f464a

Thanks for pushing on this!

@seanmonstar seanmonstar mentioned this pull request May 31, 2017
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

Successfully merging this pull request may close these issues.

2 participants