Serde support #47

radix · 2017-12-27T22:38:17Z

Implement Serialize and Deserialize for Quantity, with tests.

Fixes #37.

iliekturtles

I've never used Serde before so I still want to go through their documentation, however the initial review looks good. When you get to uom's documentation lib.rs and README.md will need the same text explaining the serde feature. I don't have a good way to update both without copy-paste-fix-formatting currently.

iliekturtles · 2017-12-28T02:21:00Z

Cargo.toml

@@ -23,6 +23,7 @@ maintenance = { status = "actively-developed" }
 [dependencies]
 num = "0.1"
 typenum = "1.9.0"
+serde = { version = "1.0", optional = true, features = ["derive"] }


Move above typenum so dependencies are in alphabetical order.

iliekturtles · 2017-12-28T02:23:16Z

src/lib.rs

@@ -155,6 +155,9 @@ compile_error!("A least one underlying storage type must be enabled. See the fea

 #[doc(hidden)]
 pub extern crate num;


Add a blank line just below num.

iliekturtles · 2017-12-28T02:25:11Z

src/system.rs

+            U: Units<V> + ?Sized,
+            V: $crate::num::Num + $crate::Conversion<V> + $crate::serde::ser::Serialize,
+        {
+            fn serialize<S: $crate::serde::ser::Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {


Put S's type bounds in a where clause instead of inline.

iliekturtles · 2017-12-28T02:26:59Z

src/system.rs

+            where
+                De: $crate::serde::de::Deserializer<'de>,
+            {
+                let value: V = $crate::serde::de::Deserialize::deserialize(deserializer)?;


Blank line beneath this one.

iliekturtles · 2017-12-28T02:37:20Z

src/lib.rs

@@ -155,6 +155,9 @@ compile_error!("A least one underlying storage type must be enabled. See the fea

 #[doc(hidden)]
 pub extern crate num;
+#[doc(hidden)]


I think this should be #[cfg_attr(feature = "serde", doc(hidden)] so that the attribute only appears when serde is enabled. Didn't test with the compiler however.

I tested running cargo doc with and without the serde feature enabled and it seemed to work fine in both cases (not showing docs for serde in either case). However, of course, if you'd like me to specify a cfg_attr anyway I can do that.

radix · 2017-12-28T06:40:16Z

I've addressed your comments, but I still need to write some tests. I should be able to get to that tomorrow! :)

coveralls · 2017-12-28T06:44:29Z

Coverage remained the same at 98.559% when pulling 8e936c4 on radix:serde into 641ebe5 on iliekturtles:master.

coveralls · 2017-12-28T15:27:27Z

Coverage remained the same at 98.559% when pulling cd81039 on radix:serde into 6745365 on iliekturtles:master.

coveralls · 2017-12-28T15:46:02Z

Coverage remained the same at 98.559% when pulling 0ad4ace on radix:serde into 6745365 on iliekturtles:master.

radix · 2017-12-28T15:49:46Z

Okay, this is probably ready for another review pass. I've implemented simple tests. Note that I've also set up travis to run the serde tests in addition to the default-feature tests.

radix · 2017-12-28T15:50:47Z

Oh, I'll also update CHANGELOG.md.

coveralls · 2017-12-28T16:00:13Z

Coverage remained the same at 98.559% when pulling ee6f509 on radix:serde into 6745365 on iliekturtles:master.

iliekturtles

Include cargo test --verbose --features serde in .appveyor.yml. Comments for README.md also apply to lib.rs.

iliekturtles · 2017-12-28T17:58:49Z

CHANGELOG.md

@@ -14,6 +14,8 @@
 ### Added
 * [#26](https://github.com/iliekturtles/uom/issues/26) Implement `num::Zero`.
 * [#35](https://github.com/iliekturtles/uom/issues/35) Implement `num::Saturating`.
+ * [#37](https://github.com/iliekturtles/uom/issues/35) Implement `serde::Serialize` and
+   `serde::Deserialize`.


Mention that this functionality is disabled by default and can be enabled with the serde feature.

iliekturtles · 2017-12-28T18:23:43Z

src/lib.rs

@@ -156,6 +159,13 @@ compile_error!("A least one underlying storage type must be enabled. See the fea
 #[doc(hidden)]
 pub extern crate num;

+#[doc(hidden)]


Looks like cfg_attr isn't necessary. The compiler is smart enough that #[doc(hidden)] doesn't "fall through" to the next item when the serde feature is not enabled.

iliekturtles · 2017-12-28T18:25:57Z

src/lib.rs

+#[cfg(feature = "serde")]
+pub extern crate serde;
+
+#[cfg(all(feature = "serde", test))]


Change order so that test appears first. #[cfg(all(test, feature = "serde"))]. I also have the dev-dependencies in a separate section beneath the regular dependencies. Not sure if this should be maintained or if all dependencies should just be included in order. Thoughts?

I'm happy to maintain the current style, I'll move it below.

iliekturtles · 2017-12-28T18:27:03Z

src/tests.rs

@@ -7,6 +7,8 @@ use num::{Float, FromPrimitive, One, Saturating, Signed, Zero};
 use quickcheck::TestResult;
 use lib::fmt::Debug;
 use lib::marker::PhantomData;
+#[cfg(all(feature = "serde", test))]


test isn't necessary since this module is only compiled when test is set.

iliekturtles · 2017-12-28T18:29:56Z

src/tests.rs

+            #[cfg(feature = "serde")]
+            #[allow(trivial_casts)]
+            fn serde_deserialize(v: A<V>) -> bool {
+                let json_f = serde_json::to_string(&*v).expect("Must be able to deserialize num");


I believe the expect(...) should say serialize and not deserialize.

Right you are!

iliekturtles · 2017-12-28T18:31:19Z

src/tests.rs

+            #[allow(trivial_casts)]
+            fn serde_serialize(v: A<V>) -> bool {
+                let m = Length::new::<meter>((*v).clone());
+                let json_q = serde_json::to_string(&m).expect("Must be able to serialize Quantity");


Put let json_q = ... after let json_f = ... to match comparison order below.

iliekturtles · 2017-12-28T18:31:36Z

src/tests.rs

+                let length: Length = serde_json::from_str(&json_f)
+                    .expect("Must be able to deserialize Quantity");
+
+                Test::approx_eq(&length.get(meter), &*v)


Make &*v the first parameter.

iliekturtles · 2017-12-28T18:33:19Z

README.md

+inclusion of the pre-built [International System of Units][si] (SI), support for [Serde][serde], and
+`no_std` functionality. The features are described below. `f32`, `f64`, `std`, and `si` are enabled
+by default. Features can be cherry-picked by using the `--no-default-features` and
+`--features "..."` flags when compiling `uom` or specifying features in Cargo.toml:


Wrap at 100 characters.

iliekturtles · 2017-12-28T18:35:03Z

README.md

@@ -88,12 +88,15 @@ uom = {
   `rational`, `rational32`, `rational64`, `bigrational`, `f32`, `f64` -- Features to enable
   underlying storage types. At least one of these features must be enabled. `f32` and `f64` are
   enabled by default.
+ * `serde` -- Feature to enable support for serialization and deserialization of quantities with
+   the [serde][serde] crate.


Note that the feature is disabled by default. Also include the serde feature in the toml block above: "serde", # Serde support.

radix · 2017-12-28T22:33:40Z

I believe I've addressed your comments, thanks for the review!

coveralls · 2017-12-28T22:35:12Z

Coverage remained the same at 98.559% when pulling a727657 on radix:serde into 6745365 on iliekturtles:master.

coveralls · 2017-12-28T22:38:28Z

Coverage remained the same at 98.559% when pulling a727657 on radix:serde into 6745365 on iliekturtles:master.

radix · 2017-12-29T00:03:11Z

FWIW, the appveyor failures are only in the old rem tests that occasionally fail, not the new serde code.

iliekturtles

With these changes I think we'll be ready to merge!

iliekturtles · 2017-12-28T23:59:49Z

.appveyor.yml

@@ -56,6 +56,7 @@ build: false
 test_script:
  - cargo build --verbose
  - cargo test --verbose


You can remove this line. The next line runs the same tests + serde tests.

That's fair. I should say that I did it this way intentionally, to make sure that the crate still builds and tests correctly without the serde feature enabled (making sure I set up my cfg attributes correctly etc). But with enough of these kinds of features it would lead to ridiculous test times, having to run through the test suite several times... so yeah, I can remove it :)

iliekturtles · 2017-12-29T00:15:08Z

CHANGELOG.md

@@ -14,6 +14,8 @@
 ### Added
 * [#26](https://github.com/iliekturtles/uom/issues/26) Implement `num::Zero`.
 * [#35](https://github.com/iliekturtles/uom/issues/35) Implement `num::Saturating`.
+ * [#37](https://github.com/iliekturtles/uom/issues/35) Implement `serde::Serialize` and
+   `serde::Deserialize`. Disabled by default. Enabled with the `serde` cargo feature.


Just say "serde feature".

iliekturtles · 2017-12-29T00:15:48Z

README.md

@@ -79,6 +79,7 @@ uom = {
        "bigint", "biguint", # Arbitrary width integer storage types.
        "rational", "rational32", "rational64", "bigrational", # Integer ratio storage types.
        "f32", "f64", # Floating point storage types.
+	"serde", # Serde support.


Looks like this line didn't get indented.

oops, that was a sneaky tab character.

iliekturtles · 2017-12-29T00:17:19Z

src/tests.rs

@@ -7,6 +7,8 @@ use num::{Float, FromPrimitive, One, Saturating, Signed, Zero};
 use quickcheck::TestResult;
 use lib::fmt::Debug;
 use lib::marker::PhantomData;
+#[cfg(all(feature = "serde"))]


all(...) isn't needed anymore.

coveralls · 2017-12-29T01:13:23Z

Coverage remained the same at 98.559% when pulling c0d8f78 on radix:serde into 6745365 on iliekturtles:master.

iliekturtles · 2017-12-29T17:14:23Z

I spoke a bit too soon about being ready to merge. I finally read through the Serde documentation and have a few more questions:

Should we be using serialize_struct or serialize_newtype_struct: https://serde.rs/impl-serialize.html#serializing-a-struct
Should we be using this instead of serde_json: https://serde.rs/unit-testing.html
https://serde.rs/feature-flags.html#-feature-std -- I believe we should always disable this feature since we don't need the impls it provides. Or possibly tie to uom's std feature.

radix · 2017-12-29T17:40:10Z

@iliekturtles no problem!

Hmm, that's interesting. so, to get the first thing out of the way -- not serialize_struct, because we're not serializing a compound structure here, only one value (the bare number in the base unit).

As for serialize_newtype_struct -- I believe what I implemented is equivalent to using serialize_newtype_struct, and honestly I'm not sure what the point of serialize_newtype_struct is. As the documentation says, implementors of serialization formats are supposed to just serialize the wrapped value, but I suppose it gives them the option of doing something different (against the advice of the documentation).
I'm honestly not sure why I should use it -- I'm currently trying to get some advice from someone who is more familiar with it. It's worth noting that our struct isn't literally a newtype, since it has those other phantom fields. Looking at the implementations of backends, it seems like serializer.serialize_newtype_struct("Foobar", &self.0) is always equivalent to self.0.serialize(serializer).
I've always just used serde_json personally, since it's simpler and more concise to use than serde_test. I think serde_test might be more useful when you want somewhat more specific tests (e.g. testing the order of serialization of fields and whatnot). However, I'd be happy to switch -- it should be simple enough.
Good point. I'll see if there are any complications...

radix · 2017-12-29T18:14:22Z

Okay, about point 1: https://github.com/ron-rs/ron serializes "serializes_newtype_struct" differently, so the name you pass is actually encoded into the output. I could also perhaps pass the name of the base unit as the newtype name, to make that output a bit more descriptive. I'll also have to deserialize with deserialize_newtype_struct. I'll make that change and let you know!

iliekturtles · 2017-12-29T19:02:14Z

Thanks for the information. Leave points 1 and 2 as-is for the moment. I'm going to do a bit more digging.

radix · 2017-12-29T19:24:16Z

@iliekturtles Just FYI I was looking into using serialize_newtype_struct -- that's easy enough, but the deserialization side is more complicated. It needs a visitor to be passed, and I was trying to figure out how to generate one of those without too much work. But okay, I'll leave that alone and get back to disabling serde's std feature -- which has its own complications, because apparently our dependency on other crates is relying on the serde std feature. I'm digging into that some more.

coveralls · 2017-12-29T19:50:18Z

Coverage remained the same at 98.559% when pulling 0b75490 on radix:serde into 6745365 on iliekturtles:master.

radix · 2017-12-29T20:26:25Z

Okay, I think I fixed the third point, but I was real confused for a while. Basically, there is a cargo bug when building uom directly with serde enabled: rust-lang/cargo#4664

Basically, because we have a dev-dependency on serde_json, which depends on serde (with default features), cargo commands run directly against uom still build serde with default features. However, when uom is used as a dependency, uom's dev-dependencies is ignored and so serde won't require std. I verified this manually by creating an example project that depends on my fork of uom, with serde enabled. I built it with --verbose and saw that it does not include serde's std feature.

radix · 2017-12-29T20:28:48Z

btw, @iliekturtles , if you want to chat about any of these things in real-time, I am on the various rust-related channels on the mozilla IRC channels as M-radix (or radix)

iliekturtles · 2017-12-29T20:43:09Z

error[E0277]: the trait bound `num::BigInt: serde::Serialize` is not satisfied
   --> src\tests.rs:537:30
    |
537 |                 let json_f = serde_json::to_string(&*v).expect("Must be able to serialize num");
    |                              ^^^^^^^^^^^^^^^^^^^^^ the trait `serde::Serialize` is not implemented for `num::BigInt`
    |
    = note: required by `serde_json::to_string`

One more issue I found when running cargo test --all-features. BigInt and other complex types don't implement serde traits by default. Looks like there are features to include this functionality: https://github.com/rust-num/num/blob/master/Cargo.toml. Note that I just pushed a fix for --all-features not compiling because of num::Saturating.

Once this is resolved I'll merge for real. I still want to look at points 1 and 2 above, but will complete that before releasing v0.17.0.

I used to be on IRC via a very old laptop that was lost to a storm this fall. I'll see about getting connected again.

radix · 2017-12-29T22:50:22Z

Ooookay. Unfortunately I think this is going to cause a bit of a pain, because of this bug: rust-lang/cargo#1286

I think what we want is to define a feature that both enables a dependency (serde) and enables features in other dependencies (num-bigint/serde, et al). But because of that linked bug, we cannot name this feature "serde", because all optional dependencies have names equal to the package name -- and sadly cargo doesn't just merge the features in the features section with the ones from the dependencies section.

So this means we need to bikeshed a name: what should it be? use_serde? enable_serde?

radix · 2017-12-29T23:15:47Z

Ah! Even more confounding problems! 😠

Apparently the num crate has not been updated to serde 1.0. So there's no hope for us to provide serde support for its data types yet. rust-num/num#357

So I suppose we should just disable the tests for those backend types for now. However, I suppose we should still standardize/document a feature like use_serde instead of serde, so that we can later add num/serde to that feature once they update their support to serde 1.0.

So I'll

add the use_serde feature and document it as the way to turn on serde support
move the tests for serde into a new submodule that has everything enabled except the num datatypes.
how about for now I also add a cargo build --all-features --tests to the travis/appveyor configs, so that we at least know the code compiles, if not passes tests?

iliekturtles · 2017-12-31T01:59:11Z

use_serde sounds good.
Move the serde tests into the op_assign module and rename to mod primitive.
Good idea. Change AppVeyor and TravisCI to cargo build --all-features --verbose. As tests start getting fixed we can add additional features until build servers are running cargo test --all-features.

Fixes iliekturtles#37.

radix · 2018-01-01T22:56:17Z

Okay, sorry for the delay @iliekturtles ! I think I've addressed the outstanding comments.

Oh, one minor difference which I'm not even sure was intentional on your part: I changed the build command to cargo build --verbose --all-features --tests instead of just cargo build --verbose --all-features. This means that the test code is at least compiled (but not necessarily run) for all features -- that's what I was originally suggesting in #47 (comment)

coveralls · 2018-01-02T00:32:46Z

Coverage remained the same at 98.559% when pulling 8ff1d87 on radix:serde into d50651e on iliekturtles:master.

iliekturtles · 2018-01-02T17:59:57Z

Merged, thanks! I ended up fixing a formatting issue in tests.rs and re-organizing some comments. Also changed build script back to cargo build --all-features. Including --tests causes Cargo to do nothing for some reason (https://travis-ci.org/iliekturtles/uom/jobs/324219347#L578).

radix · 2018-01-02T19:11:01Z

Hmm, well that's definitely strange. As you can see, other builds do show output, even from the same travis run as you linked. I wonder if there is just some caching going on that's causing cargo to realize it has nothing to do.

iliekturtles · 2018-01-03T17:42:58Z

I get the same thing locally which is where I found the issue originally:

uom (master)$ cargo clean && cargo build --all-features --tests
    Finished dev [unoptimized + debuginfo] target(s) in 0.0 secs

radix force-pushed the serde branch 3 times, most recently from 37554e0 to 2961a75 Compare December 27, 2017 23:10

iliekturtles reviewed Dec 28, 2017

View reviewed changes

radix force-pushed the serde branch from 2961a75 to 8e936c4 Compare December 28, 2017 06:38

radix force-pushed the serde branch from 8e936c4 to cd81039 Compare December 28, 2017 15:22

radix force-pushed the serde branch from cd81039 to 0ad4ace Compare December 28, 2017 15:41

radix changed the title ~~WIP: Serde support~~ Serde support Dec 28, 2017

radix force-pushed the serde branch from 0ad4ace to ee6f509 Compare December 28, 2017 15:55

iliekturtles reviewed Dec 28, 2017

View reviewed changes

radix force-pushed the serde branch 2 times, most recently from faca99e to a727657 Compare December 28, 2017 22:32

iliekturtles reviewed Dec 29, 2017

View reviewed changes

radix force-pushed the serde branch from a727657 to c0d8f78 Compare December 29, 2017 01:08

radix force-pushed the serde branch from c0d8f78 to 0b75490 Compare December 29, 2017 19:44

Implement serde::Serialize and serde::Deserialize.

8ff1d87

Fixes iliekturtles#37.

radix force-pushed the serde branch from 0b75490 to 8ff1d87 Compare January 1, 2018 22:44

iliekturtles added a commit that referenced this pull request Jan 2, 2018

Merge pull request #47 from radix/serde

0b33222

iliekturtles closed this Jan 2, 2018

		@@ -155,6 +155,9 @@ compile_error!("A least one underlying storage type must be enabled. See the fea

		#[doc(hidden)]
		pub extern crate num;

Serde support #47

Serde support #47

Conversation

radix commented Dec 27, 2017 • edited Loading

iliekturtles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radix commented Dec 28, 2017

coveralls commented Dec 28, 2017

coveralls commented Dec 28, 2017

coveralls commented Dec 28, 2017

radix commented Dec 28, 2017

radix commented Dec 28, 2017

coveralls commented Dec 28, 2017

iliekturtles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radix commented Dec 28, 2017

coveralls commented Dec 28, 2017

coveralls commented Dec 28, 2017

radix commented Dec 29, 2017

iliekturtles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 29, 2017

iliekturtles commented Dec 29, 2017

radix commented Dec 29, 2017 • edited Loading

radix commented Dec 29, 2017

iliekturtles commented Dec 29, 2017

radix commented Dec 29, 2017

coveralls commented Dec 29, 2017

radix commented Dec 29, 2017

radix commented Dec 29, 2017

iliekturtles commented Dec 29, 2017

radix commented Dec 29, 2017

radix commented Dec 29, 2017

iliekturtles commented Dec 31, 2017

radix commented Jan 1, 2018

coveralls commented Jan 2, 2018

iliekturtles commented Jan 2, 2018

radix commented Jan 2, 2018

iliekturtles commented Jan 3, 2018

radix commented Dec 27, 2017 •

edited

Loading

radix commented Dec 29, 2017 •

edited

Loading