feat(serde): add support for Serde serialization

nickel-org / rust-mustache

mustache template library for rust

Other

202 stars 61 forks source link

feat(serde): add support for Serde serialization #40

Closed Ryman closed 7 years ago

Ryman commented 8 years ago

To use this feature, you can activate the serde feature:

[dependencies.mustache]
version = "0.8"
features = ["serde"]

The minimum compiler version for the serde feature will track stable.

Closes #33.

Ryman commented 8 years ago

r? @KodrAus

Specific review concerns:

I'm sure there's probably neater ways to do some of the cfg stuff
I'm unsure if re-exporting both traits as Encodable makes any sense but it simplifies the imports and usage within this crate so hopefully it's not confusing for users?

Still TODO if I get to it: more tests for serialization. This passes what we currently have but it's could be more exhaustive on our 'compatible types'. Any specific edge case suggestions would definitely be appreciated!

KodrAus commented 8 years ago

I think re-exporting both traits as the same name makes sense, since other libraries that only want to maintain one or the other can still satisfy the type bound by implementing rustc_serialize::Encodable or serde::Serialize (I'm pretty sure).

I'll pull this down to my nickel branch and see how it all plays :) Looks good to me though!

dtolnay commented 7 years ago

What is the status of this? Waiting on anything?

KodrAus commented 7 years ago

Well this would need to be updated for serde 0.9. Looking at this again I was going to test this branch in my nickel PR. I did end up doing that, and it all worked out well. I didn't come back and report that though, so I think I dropped the ball.

I'd be happy to see it start moving again though.

petehayes102 commented 7 years ago

It'd be great to have some movement on this. Especially considering rustc-serialize is planned to be deprecated in 1.18: https://internals.rust-lang.org/t/rust-release-milestone-predictions/4591

erickt commented 7 years ago

FYI, I've been working on updating this to work with serde 1.0.

Ryman commented 7 years ago

Closing in favor of #52