Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upUpdate to serde 0.9.X #93
Conversation
| } | ||
| } | ||
|
|
||
| impl<'a, R: Read + 'a> serde::de::VariantVisitor for VariantVisitor<'a, R> { |
This comment has been minimized.
This comment has been minimized.
dtolnay
Jan 31, 2017
Collaborator
We already have a VariantVisitor below don't we? Let's use that one.
impl<'a, R: Read> serde::de::VariantVisitor for &'a mut Deserializer<R> { /* ... */ }| { | ||
| visitor.visit(self) | ||
| } | ||
| struct VariantVisitor<'a, R: Read + 'a>(&'a mut Deserializer<R>); |
This comment has been minimized.
This comment has been minimized.
dtolnay
Jan 31, 2017
Collaborator
I think this would be clearer without the wrapper type:
impl<'a, R: Read> serde::de::EnumVisitor for &'a mut Deserializer<R>|
|
||
| fn end_of_stream() -> DeserializeError { | ||
| DeserializeError::Serde(serde::de::value::Error::EndOfStream) | ||
| fn custom<T: ::std::fmt::Display>(desc: T) -> DeserializeError { |
This comment has been minimized.
This comment has been minimized.
dtolnay
Jan 31, 2017
Collaborator
The fmt module is already imported above so this could be fmt::Display instead of ::std::fmt::Display.
| } | ||
|
|
||
| fn wrap_io(err: IoError) -> SerializeError { | ||
| SerializeError::IoError(err) | ||
| } | ||
|
|
||
| impl serde::ser::Error for SerializeError { | ||
| fn custom<T: Into<String>>(msg: T) -> Self { | ||
| SerializeError::Custom(msg.into()) | ||
| fn custom<T: ::std::fmt::Display>(msg: T) -> Self { |
This comment has been minimized.
This comment has been minimized.
| fn custom<T: Into<String>>(msg: T) -> Self { | ||
| SerializeError::Custom(msg.into()) | ||
| fn custom<T: ::std::fmt::Display>(msg: T) -> Self { | ||
| SerializeError::Custom(format!("{}", msg)) |
This comment has been minimized.
This comment has been minimized.
dtolnay
Jan 31, 2017
Collaborator
I think this would be clearer as msg.to_string() instead of format!("{}", msg).
This comment has been minimized.
This comment has been minimized.
ZoeyR
Jan 31, 2017
Author
Collaborator
Should I make these updates as separate commits? or squash all the changes into one commit?
This comment has been minimized.
This comment has been minimized.
dtolnay
Jan 31, 2017
Collaborator
Please make any changes as new commits. GitHub's code review has very poor support for reviewing squashed / amended / rebased commits so I would have to review the whole thing all over again to see what you changed. If he prefers, @TyOverby can squash or rebase the whole thing at the end when the review is done and it is ready to merge.
|
Regarding the error tests - I am not sure why |
|
Should I re-add the test and change |
|
Yes please change the error to a String and update the deserializing_errors test to check the error messages. |
Reintroduce error tests.
| @@ -54,7 +54,7 @@ impl Error for DeserializeError { | |||
| DeserializeError::IoError(ref err) => Error::description(err), | |||
| DeserializeError::InvalidEncoding(ref ib) => ib.desc, | |||
| DeserializeError::SizeLimit => "the size limit for decoding has been reached", | |||
| DeserializeError::Serde(ref s) => s.description(), | |||
| DeserializeError::Custom(_) => "a custom serialization error was reported", | |||
This comment has been minimized.
This comment has been minimized.
dtolnay
Jan 31, 2017
Collaborator
You should be able to return the real error here. DeserializeError::Custom(ref msg) => msg
| @@ -77,7 +77,7 @@ impl From<IoError> for DeserializeError { | |||
|
|
|||
| impl From<serde::de::value::Error> for DeserializeError { | |||
This comment has been minimized.
This comment has been minimized.
| DeserializeError::Serde(ref s) => | ||
| s.fmt(fmt), | ||
| DeserializeError::Custom(ref s) => | ||
| write!(fmt, "Custom Error {}", s), |
This comment has been minimized.
This comment has been minimized.
dtolnay
Jan 31, 2017
Collaborator
I would prefer to just format the message like before - s.fmt(fmt). I don't think "Custom Error" adds any clarity or context.
This comment has been minimized.
This comment has been minimized.
ZoeyR
Jan 31, 2017
Author
Collaborator
Sounds good. I was just following the convention used by SerializeError
This comment has been minimized.
This comment has been minimized.
dtolnay
Jan 31, 2017
Collaborator
A major focus of serde 0.9 is better error message so as the person doing this upgrade, part of your job is improving existing bad error messages!
|
I tried compiling this and I get a ton of unused_import, unused_mut, and unused_variables warnings. The master branch builds cleanly so please fix all of the warnings. |
|
I wasn't aware those errors were introduced with my changes. I'll get those corrected. |
|
This is really great work @dgriffen, thanks for taking this PR on! From the bincode API side, this all looks good to me. @dtolnay: is there anything remaining as far as integration with serde is concerned? |
|
Looks good to me. |
|
|
|
@TyOverby are there any more blocking issues that a new release is waiting on? I'd be willing to work on them. |
|
Possibly #86. |
|
@dgriffen: I also just published |
ZoeyR commentedJan 31, 2017
•
edited
Closes #92
One of the few questions I had left after doing this was what to do with the
isize_invalid_deserializetest. The test used variants from theserde::de::value::Errorenum. This type is now a struct and doesn't have variants anymore so the test wouldn't work without string parsing, which seemed fragile.