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 uphandle json serialization errors #129
Conversation
little-dude
added some commits
May 31, 2017
KodrAus
reviewed
Jun 1, 2017
|
Thanks for jumping on this @little-dude! I've just left a few of my thoughts, but @seanmonstar might have differing opinions. |
| pub fn json<T: Serialize>(mut self, json: &T) -> RequestBuilder { | ||
| let body = serde_json::to_vec(json).expect("serde to_vec cannot fail"); | ||
| pub fn json<T: Serialize>(mut self, json: &T) -> ::Result<RequestBuilder> { | ||
| let body = serde_json::to_vec(json).map_err(::error::from)?; | ||
| self.headers.set(ContentType::json()); | ||
| self.body = Some(Ok(body.into())); |
This comment has been minimized.
This comment has been minimized.
KodrAus
Jun 1, 2017
Contributor
We should be able to make RequestBuilder.body an Option<Body> now instead of Option<::Result<Body>>.
This comment has been minimized.
This comment has been minimized.
seanmonstar
Jun 1, 2017
Owner
Also requires an update in the form method, I've got a branch working on adjusting the builder to be &mut self.
| @@ -612,16 +612,16 @@ impl RequestBuilder { | |||
| /// | |||
| /// let client = reqwest::Client::new()?; | |||
| /// let res = client.post("http://httpbin.org") | |||
| /// .json(&map) | |||
| /// .json(&map)? | |||
| /// .send()?; | |||
| /// # Ok(()) | |||
| /// # } | |||
| /// ``` | |||
This comment has been minimized.
This comment has been minimized.
KodrAus
Jun 1, 2017
Contributor
Now that the json method is fallible we should add an Errors section to the docs. Here's the relevant API guideline with some inspiration.
This comment has been minimized.
This comment has been minimized.
little-dude
Jun 1, 2017
Author
Contributor
I think I can reuse the explanation that is in serde_json for this.
| let some_url = "https://google.com/"; | ||
| let r = client.post(some_url); | ||
| let json_data = MyStruct{}; | ||
| assert_eq!(format!("{}", r.json(&json_data).unwrap_err()), "nope".to_string()); |
This comment has been minimized.
This comment has been minimized.
KodrAus
Jun 1, 2017
Contributor
I would just check the error kind here rather than relying on the format:
assert!(r.json(&bad_json).unwrap_err().is_serialization());
This comment has been minimized.
This comment has been minimized.
little-dude
Jun 1, 2017
Author
Contributor
Done. It's looks cleaner. Is that the only reason you'd do it that way? (just curious)
little-dude
added some commits
Jun 1, 2017
seanmonstar
merged commit f3df667
into
seanmonstar:master
Jun 1, 2017
This comment has been minimized.
This comment has been minimized.
|
Thanks! |
little-dude commentedMay 31, 2017
This is an attempt at fixing #112