Skip to content
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

Merged
merged 4 commits into from
Jun 21, 2018
Merged

Conversation

adeschamps
Copy link
Contributor

@adeschamps adeschamps commented Jan 18, 2018

Implementing #31

This turned out to be pretty simple to implement, although I do have a few questions:

  • Did I get the feature flags right? Should serde_test be a dev-dependency, or does it not really matter since it's optional?
  • When running $ 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?

@saethlin
Copy link

Isn't this dimensionally unsafe? That is, doesn't this allow me to serialize some Meter3 and then deserialize some PerMeter3 from it?

@adeschamps
Copy link
Contributor Author

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.

@droundy
Copy link
Contributor

droundy commented Jan 30, 2018

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.

@adeschamps
Copy link
Contributor Author

@paholg Have you had a chance to look at this?

@paholg
Copy link
Owner

paholg commented Mar 10, 2018

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 serde feature anywhere in the dependency chain, that prevents someone from defining it a different way (such as serializing units as well).

@adeschamps
Copy link
Contributor Author

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 make_units macro so that one can opt in to defining serde traits. That way, the built in systems can have serde traits defined (if the feature is enabled), and someone who's making their own system can either use the default or define their own.

Serde traits are enabled for all built in unit systems (as long as the
serde feature is enabled)
@adeschamps
Copy link
Contributor Author

Here's a modified version. I added a pattern, serde = true; or serde = false;, to the make_units! macro. If set to true, then serde traits will be implemented as long as the serde feature is enabled. All of the built in unit systems have this set to true.

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 make_units! macro.

@adeschamps
Copy link
Contributor Author

@paholg Do you have any more thoughts on this?

@droundy
Copy link
Contributor

droundy commented Jun 19, 2018

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.

@paholg
Copy link
Owner

paholg commented Jun 20, 2018

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 make_units! macro, would you mind putting it in a separate macro (and then still calling that macro for all the builtin unit systems).

That way, we also avoid the breaking change.

This lets us implement serde traits without making a breaking change
to the make_units! macro.
@adeschamps
Copy link
Contributor Author

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 cargo test --features "serde serde_test" succeeds.

@paholg paholg merged commit a946893 into paholg:master Jun 21, 2018
@paholg
Copy link
Owner

paholg commented Jun 21, 2018

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.

@paholg paholg mentioned this pull request Jun 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants