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 Response::text()? #86

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

Add Response::text()? #86

theduke opened this issue May 8, 2017 · 8 comments
Labels
B-rfc Blocked: Request for comments. More discussion would help move this along.

Comments

@theduke
Copy link
Contributor

theduke commented May 8, 2017

Currently, you can run into a (somewhat) surprising condition when you read the body of a Response twice.

  • `resp.read_to_string(&mut s);
  • resp.json()

or just

  • `resp.read_to_string(&mut s);
  • `resp.read_to_string(&mut s);

While to many it will occur quickly that the problem is reading the response twice, I think this can be improved.

.json() already consumes the response, but the Read trait methods do not.

Maybe there could be a body() method which returns a Read and also consumes the response.

@seanmonstar
Copy link
Owner

The returned Read would have the same "issue", that you can try to read from it multiple times. This is just the design in Rust. For things where you can rewind, there is the Seek trait, and Response doesn't implement it, because you can't rewind.


Alternatively, since it seems to be so common to read the body to a string, I wonder if there is value in providing response.string()...

@theduke
Copy link
Contributor Author

theduke commented May 8, 2017

The returned Read would have the same "issue", that you can try to read from it multiple times.

Good point.

@imp
Copy link
Contributor

imp commented May 17, 2017

response.text() would be my choice. In fact that is what I have in requests

@cramertj
Copy link

+1 for a .string() or similar.

@seanmonstar seanmonstar changed the title Improve Api Design: Consuming response body Add Response::text()? Jun 8, 2017
@jaemk
Copy link
Contributor

jaemk commented Jun 9, 2017

I can add this in

@seanmonstar
Copy link
Owner

I currently have it here in question form as I'd like to hear if it's worth having, or to continue to recommend the io::Read way of read_to_string(&mut s)...

@jaemk
Copy link
Contributor

jaemk commented Jun 9, 2017

I see. I think it's worth having since it's probably the most likely case other than deserializing into json. The convenience and the consumption of the response is certainly a plus.

@cramertj
Copy link

cramertj commented Jun 9, 2017

One argument in favor of adding the method: without this, you have to use std::io::Read;, let mut s = String::new();, and only then can you call read_to_string(&mut s). That's much less convenient than .text().

@seanmonstar seanmonstar added the B-rfc Blocked: Request for comments. More discussion would help move this along. label Aug 19, 2017
repi pushed a commit to EmbarkStudios/reqwest that referenced this issue Dec 20, 2019
* fix: Add new patterns for backtrace cleaning

* feat: Use fuzzy matching for well known system frame detection
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-rfc Blocked: Request for comments. More discussion would help move this along.
Projects
None yet
Development

No branches or pull requests

5 participants