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 upSupport serde 0.7 #60
Conversation
|
Since power comes with responsibility, need a review? :) |
|
@nox That would be great! Also, do you know when servo plans on moving to serde-0.7? |
|
There is a Webrender workweek ongoing so we are waiting until it finishes to not make their work harder. So next week probably, when we will do a rustup. |
| @@ -1,6 +1,6 @@ | |||
| [package] | |||
| name = "bincode" | |||
| version = "0.4.0" | |||
| version = "0.4.1" | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
TyOverby
Mar 3, 2016
Author
Collaborator
Yep. I don't know how this sneaked in. 0.4.1 was the release that fixed the dependencies. This PR is certainly a breaking change.
| EndOfStreamError, | ||
| UnknownFieldError, | ||
| MissingFieldError, | ||
| EndOfStream, |
This comment has been minimized.
This comment has been minimized.
nox
Mar 3, 2016
Member
serde::de::value::Error has a EndOfStreamError, I don't think there is any need to duplicate it.
This comment has been minimized.
This comment has been minimized.
TyOverby
Mar 3, 2016
Author
Collaborator
That's true, but outside of serde, you have ByteOrderError::UnexpectedEndOfStream. I thought it would be weird to coerce a ByteOrderError into a serde::de::value::Error, but I could be convinced otherwise.
This comment has been minimized.
This comment has been minimized.
nox
Mar 3, 2016
Member
We can very well special case DeserializeError::Serde(de::value::Error::EndOfStreamError), no? I find it weird to have two different EndOfStream errors.
This comment has been minimized.
This comment has been minimized.
| @@ -506,7 +481,8 @@ impl<'a, R: Read> serde::de::VariantVisitor for Deserializer<'a, R> { | |||
| { | |||
| let index: u32 = try!(serde::Deserialize::deserialize(self)); | |||
| let mut deserializer = (index as usize).into_deserializer(); | |||
| Ok(try!(serde::Deserialize::deserialize(&mut deserializer))) | |||
| let attempt: Result<V, serde::de::value::Error> = serde::Deserialize::deserialize(&mut deserializer); | |||
| Ok(try!(attempt)) | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
nox
Mar 4, 2016
Member
You should be able to do:
Ok(try!(serde::Deserialize::deserialize::<Result<V, _>>(&mut deserializer)))I'm not sure it's an improvement though.
|
Apart from these few remarks, LGTM! |
|
As soon as the lights go green I'll merge! |
TyOverby commentedFeb 29, 2016
Closes #55