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

rust_serialize -> serde #24

Merged
merged 2 commits into from Jul 16, 2017

Conversation

Projects
None yet
2 participants
@Eternity-Yarr
Contributor

Eternity-Yarr commented Jul 15, 2017

Something like this for #17

@conradkdotcom

This comment has been minimized.

Show comment
Hide comment
@conradkdotcom

conradkdotcom Jul 16, 2017

Owner

@Eternity-Yarr Wow, thanks so much for taking the time to look at this ! ❤️ 😃

Just compiled and tested this locally. It seems to work fine on its own. But it is not currently backward compatible with Rooster files from versions <=2.6.0, because of the removal of the Encodable and Decodable traits from src/safe_string.rs.

For a bit of background, these trait implementations allowed the SafeString to appear as a regular string in the JSON, ie {"password": "blabla"} instead of {"password":{"inner":"blabla"}}.

We would probably have to rewrite the Encodable/Decodable implements with serde's Serialize and Deserialize traits.

Would you mind having a look at this please ? If not, that's OK too, just let us know in a comment 😉

Thanks again !

Owner

conradkdotcom commented Jul 16, 2017

@Eternity-Yarr Wow, thanks so much for taking the time to look at this ! ❤️ 😃

Just compiled and tested this locally. It seems to work fine on its own. But it is not currently backward compatible with Rooster files from versions <=2.6.0, because of the removal of the Encodable and Decodable traits from src/safe_string.rs.

For a bit of background, these trait implementations allowed the SafeString to appear as a regular string in the JSON, ie {"password": "blabla"} instead of {"password":{"inner":"blabla"}}.

We would probably have to rewrite the Encodable/Decodable implements with serde's Serialize and Deserialize traits.

Would you mind having a look at this please ? If not, that's OK too, just let us know in a comment 😉

Thanks again !

@Eternity-Yarr

This comment has been minimized.

Show comment
Hide comment
@Eternity-Yarr

Eternity-Yarr Jul 16, 2017

Contributor

Isn't it derived automagically? Ah.. sorry, i've got it. haven't read edited version of this comment. Sorry.

Contributor

Eternity-Yarr commented Jul 16, 2017

Isn't it derived automagically? Ah.. sorry, i've got it. haven't read edited version of this comment. Sorry.

@conradkdotcom

This comment has been minimized.

Show comment
Hide comment
@conradkdotcom

conradkdotcom Jul 16, 2017

Owner

@Eternity-Yarr Yes, I did edit my comment when I realised it missed some useful information. Sorry for any confusion. So to answer, SafeString is not derived correctly with the attributes #[...] since the JSON should not contain inner. I hope this clears things up 😃

Owner

conradkdotcom commented Jul 16, 2017

@Eternity-Yarr Yes, I did edit my comment when I realised it missed some useful information. Sorry for any confusion. So to answer, SafeString is not derived correctly with the attributes #[...] since the JSON should not contain inner. I hope this clears things up 😃

@Eternity-Yarr

This comment has been minimized.

Show comment
Hide comment
@Eternity-Yarr

Eternity-Yarr Jul 16, 2017

Contributor

@conradkdotcom well, how about that? :)

Contributor

Eternity-Yarr commented Jul 16, 2017

@conradkdotcom well, how about that? :)

@Eternity-Yarr

This comment has been minimized.

Show comment
Hide comment
@Eternity-Yarr

Eternity-Yarr Jul 16, 2017

Contributor

(forgot to remove unwrap call)

Contributor

Eternity-Yarr commented Jul 16, 2017

(forgot to remove unwrap call)

@conradkdotcom

This comment has been minimized.

Show comment
Hide comment
@conradkdotcom

conradkdotcom Jul 16, 2017

Owner

Looking good @Eternity-Yarr ! Thanks a lot for working on this so quickly ! I'll give it a go locally and keep you updated in the coming minutes 👍

Owner

conradkdotcom commented Jul 16, 2017

Looking good @Eternity-Yarr ! Thanks a lot for working on this so quickly ! I'll give it a go locally and keep you updated in the coming minutes 👍

@conradkdotcom conradkdotcom merged commit c88f43c into conradkdotcom:master Jul 16, 2017

@conradkdotcom

This comment has been minimized.

Show comment
Hide comment
@conradkdotcom

conradkdotcom Jul 16, 2017

Owner

Seems to work perfectly well ! Thanks a lot @Eternity-Yarr ! And thanks for the unit tests ! ❤️

I will publish to Crates.io soon and update the install script soon !

Owner

conradkdotcom commented Jul 16, 2017

Seems to work perfectly well ! Thanks a lot @Eternity-Yarr ! And thanks for the unit tests ! ❤️

I will publish to Crates.io soon and update the install script soon !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment