-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add serde support behind a feature flag. #32
Conversation
Isn't this dimensionally unsafe? That is, doesn't this allow me to serialize some Meter3 and then deserialize some PerMeter3 from it? |
Yes, and intentionally so. Storing the dimensions along with the value would at least double the size of the serialized data. I don't think it's worth the trade-off. |
I'd add here that serde is already type-unsafe in many or most binary formats. So there is not really much in the way of unsafety added. |
@paholg Have you had a chance to look at this? |
Hey, @adeschamps, sorry I haven't responded earlier. Right now, anyone can create a unit system and define their own serialization for it. It's also easy to convert to/from that system to one that dimensioned provides. I'm concerned that, with this addition, if dimensioned is used with the |
Sorry it's taken me so long to respond as well... If I'm understanding you correctly, the concern isn't so much that people would have different expectations about how the built in systems are serialized (I'm mainly interested in SI), but rather that someone creating their own system would also want to define how it's serialized. If that's the case, then perhaps I could modify the |
Serde traits are enabled for all built in unit systems (as long as the serde feature is enabled)
Here's a modified version. I added a pattern, If someone is defining their own unit system then they can either opt in to the default implementations (which only consider the numeric value, and ignore units), or they can opt out and decide whether they want to implement serialization themselves. This is a breaking change to the |
@paholg Do you have any more thoughts on this? |
I've just started experimenting with using dimensioned, and I'm finding that having serde support would be really nice, since it'd let me derive deserialization for my data structures. |
Hi @adeschamps. Sorry for the long delay, I somehow missed the notification that you had updated this. Thank you for making those changes. Instead of adding more to the already cumbersome That way, we also avoid the breaking change. |
This lets us implement serde traits without making a breaking change to the make_units! macro.
No worries, here it is! Travis is set up to build and test with only default features on stable and beta, so this isn't being covered by CI. The tests on nightly are failing for unrelated reasons, but I can confirm that running |
Thanks, @adeschamps! I have a separate PR in progress to try to get the travis build working again, and hopefully more consistently, so don't worry about that. |
Implementing #31
This turned out to be pretty simple to implement, although I do have a few questions:
$ cargo test --feature "serde serde_test"
, one of the doc tests fails. It's not clear to me why, since the error originates in some generated code. What can I do about that?