Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upResponse doc improvements #141
Conversation
seanmonstar
requested changes
Jun 8, 2017
|
Thanks so much for jumping on this, @jaemk! I left some pointers on how we can skip some network calls when running the doctests, and also some ideas I had on possible examples that may be useful. Let me know what you think! |
| /// assert_eq!(resp.url(), &url); | ||
| /// # Ok(()) | ||
| /// # } | ||
| /// # fn main() { |
This comment has been minimized.
This comment has been minimized.
seanmonstar
Jun 8, 2017
Owner
This will end up making the doctests make an actual network request, which is a bummer. However! This can be changed to a smaller amount of hidden code, and still show what we want.
/// # fn run() -> Result<(), Box<::std::error::Error>> {
/// let res = reqwest::get("http://httpbin.org/")?;
/// assert_eq!(res.url().as_str(), "http://httpbin.org/");
/// # }
That's all! Also, thinking about the use of checking the url of the Response, this example also could be useful to show how you can see what the final destination url was:
/// # fn run() -> Result<(), Box<::std::error::Error>> {
/// let res = reqwest::get("http://httpbin.org/redirect/1")?;
/// assert_eq!(res.url().as_str(), "http://httpbin.org/get");
/// # }
This comment has been minimized.
This comment has been minimized.
jaemk
Jun 8, 2017
Author
Contributor
Ah jeez, didn't even cross my mind to not call the run functions! I'll clean this pr up and then do another to get rid of network calls in the rest of the doc tests.
| /// let url = resp.headers().get::<Location>(); | ||
| /// println!("Received redirect to: {:?}", url); | ||
| /// } | ||
| /// status => bail!("Http Error, Response status: {:?}", status), |
This comment has been minimized.
This comment has been minimized.
seanmonstar
Jun 8, 2017
Owner
I think we may want to not show using the bail! macro in examples, or one might assume it's a macro from reqwest.
This comment has been minimized.
This comment has been minimized.
jaemk
Jun 8, 2017
Author
Contributor
Yeah that's probably a good idea. I'll see what I can do about removing error-chain requirements/usage.
| /// let resp = client.post("http://httpbin.org/post")?.send()?; | ||
| /// match *resp.status() { | ||
| /// StatusCode::Ok => println!("All good!"), | ||
| /// StatusCode::Found => { |
This comment has been minimized.
This comment has been minimized.
seanmonstar
Jun 8, 2017
Owner
reqwest will follow redirects in most cases. What if we show a very common thing to do: check if the code was a 404 (StatusCode::NotFound)?
If we wanted to include multiple examples, I could imagine some others too:
- Check for
resp.status() == StatusCode::Ok,println!("response is not ok")if not. - Make use of the
is_*methods of aStatusCode, to be more flexible.if resp.status().is_success(), orif resp.status().is_server_error(), etc. - If someone needed to inspect for a very specific status code, like
StatusCode::PayloadTooLarge(413), when trying to upload a file.
Would showing examples in that order, from most common to more niche/advanced, be useful?
| /// }; | ||
| /// # Ok(()) | ||
| /// # } | ||
| /// # fn main() { |
This comment has been minimized.
This comment has been minimized.
seanmonstar
Jun 8, 2017
Owner
Same comment about previous main in example. If we skip using error_chain, and thus don't need the extern crate, the example can just be in a plain # fn run() -> Result<(), Box<::std::error::Error>>.
| /// let content_type = resp.headers().get::<ContentType>(); | ||
| /// if let Some(ct) = content_type { | ||
| /// match **ct { | ||
| /// Mime(TopLevel::Image, SubLevel::Png, _) => { |
This comment has been minimized.
This comment has been minimized.
seanmonstar
Jun 8, 2017
Owner
Having just submitted a redesign of the mime crate, seeing it's current (v0.2) API still makes me sad. Checking the content-type is definitely something useful... but I do kind of wish we could keep that verbose mess out of here... (know I'm complaining about my own work, the mime crate, not your example).
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
seanmonstar
Jun 8, 2017
Owner
I've released a v0.3, which unfortunately we can't really use yet, since it'd require breaking hyper, which can't happen. It's in the new (not yet released) 0.11 of hyper.
Basically you can either compare with the mime directly, or via parts. So, either content_type.0 == mime::IMAGE_PNG, if you wanted an exact match, or with a match if you wanted to ignore parameters or something:
match (content_type.type_(), content_type.subtype()) {
(mime::IMAGE, mime::PNG) => (),
_ => (),
}No more importing all the different types. But as I said, we can't change this yet, so it's more just me complaining to myself...
This comment has been minimized.
This comment has been minimized.
| /// # } | ||
| /// # fn run() -> Result<()> { | ||
| /// let client = Client::new()?; | ||
| /// let mut resp = client.get("https://img.shields.io/crates/v/mime.png?label=mime")?.send()?; |
This comment has been minimized.
This comment has been minimized.
seanmonstar
Jun 8, 2017
Owner
A super simple first example could be to check the ContentLength, so as to be able to properly pre-allocate a buffer for it. Probably also best to show checking for a maximum size, so people aren't blindly downloading gigabytes accidentally.
This comment has been minimized.
This comment has been minimized.
jaemk
Jun 8, 2017
Author
Contributor
I think a simpler example like checking ContentLength would be best. The clutter of matching against the mimetype is definitely a useful example, but might be better in a standalone example.
This comment has been minimized.
This comment has been minimized.
|
Thanks for reviewing @seanmonstar! I'll get to this later tonight. |
jaemk
force-pushed the
jaemk:response_doc
branch
from
2e5325d
to
cef63db
Jun 9, 2017
This comment has been minimized.
This comment has been minimized.
|
docs are updated! Note, I rebased on master and inherited the failing tests |
jaemk
force-pushed the
jaemk:response_doc
branch
from
cef63db
to
3de6f24
Jun 9, 2017
This comment has been minimized.
This comment has been minimized.
|
This looks great! Thanks! |
jaemk commentedJun 8, 2017
•
edited by seanmonstar
urlthan just asserting it hasn't been modified...headersexample, I have the file operations commented out so I didn't need to pull in an extratempdirdev dependency. You can uncomment the three file-op lines to test that they work, but I didn't want to leave that in there to be run since it's silently modifying the project directory.Closes #115