Modified serilization to support units saved as IComparable #200

eriove · 2016-10-28T11:49:30Z

I had a class that I needed to serialize that had a unit as a IComparable. This pull requests makes this possible. I've tried testing other alternatives to not break anything. Let me know if you can think of any other use cases that should be tested.

angularsen

This is an interesting idea and I can understand why this would be convenient. I am a little concerned we are putting too much magic in the serializer though, but I'm not too familiar with what are common practices in custom serializers and json.net. I personally would be a little afraid to rely on the type names for serialization, due to possible future breaking changes out of my control and how to deal with that, but I suppose that is up to the application to decide.

If I understand correctly, one must explicitly enable TypeNameHandling.Object/All/Auto both when serializing and deserializing in order for this to work, and then it "just works".

Can you extract the serializer setup code so that it is more clear that the test cases share identical serializer and deserializer settings?
Is there a reason for not using TypeNameHandling.Auto in both serialization and deserialization? From the docs it seems a reasonable one to use.
Could you add a test case where you serialize an object with three properties, of the same types you use in the existing cases?
Could you add a test case with a non-UnitsNet type implementing IComparable, that also has Unit and Value properties?

angularsen · 2016-10-30T07:27:56Z

UnitsNet.Serialization.JsonNet/UnitsNetJsonConverter.cs

+                objectType == typeof(ValueUnit) || 
+                // All unit types implement IComparable
+                objectType==typeof(IComparable) ||
+                objectType.FullName.StartsWith("System." + nameof(IComparable)));


Why is this string comparison required, doesn't the typeof check cover all cases?

This was very much trial and error and a later refactoring made this unnecessary.

eriove · 2016-10-31T15:38:33Z

TypeNameHandling.Auto is needed for Json.Net to serialize the type information. By doing that no extra code is needed to handle deserialization of properties with interfaces. This is very convenient in combination with a plugin framework where each plugin requires it's own setting (implementing the same basic interface).

I've extracted the serializer code. Let me know if it is clear enough.

After refactoring my tests I realized that I didn't need TypeNameHandling.All.

Added the requested test cases, trying all different combinations with 3 IComparable.

Added the non UnitsNet IComparable.

Also added some nameof() to decrease the risk of name changes breaking the serialization. This could be done for more properties but that would require a specific unit to be referenced so I wasn't sure if that was the way to go.

angularsen

Starting to look good, a couple of things still.

angularsen · 2016-10-31T21:45:39Z

UnitsNet.Serialization.JsonNet.Tests/UnitsNetJsonConverterTests.cs

+                }
+            }
+
+            private static JsonSerializerSettings CreateJsonSerializerSettigns()


angularsen · 2016-10-31T21:48:44Z

UnitsNet.Serialization.JsonNet.Tests/UnitsNetJsonConverterTests.cs

+                Assert.That(deserializedTestObject.Unit, Is.EqualTo("Test"));
+            }
+
+            [Test, TestCaseSource(nameof(TestObjectsForThreeObjectsInIComparableWithDifferentValues_ExpectAllCorrectlyDeserialized))]


This part was slightly confusing to me, as I haven't used TestCaseSource before. If there is only one case, I think I would prefer a normal method that give me that TestObjWithThreeIComparable object.

This will actually be 27 tests (testing all combinations of objects). Might be a little too much. I started with 3 very similar tests and did this to reduce code duplication and then it was easy to add the loop with all combinations. I have to admit that I don't fully understand the serialization so I rather have some extra tests just to be sure.

Ah, ok. 27 tests seem a bit overboard, yes. Tests are nice, but I want each test to bring value and from the way I understand the implementation design and the test, I don't see how testing multiple variations of TestObjWithThreeIComparable will help us avoid bugs here. I think I'd rather have one or possibly two test cases with different combinations, but that should be quite enough, I think.

Their docs helped me understand what goes on: http://www.newtonsoft.com/json/help/html/T_Newtonsoft_Json_TypeNameHandling.htm

Basically they add a $type prop that describes the actual object type when it was serialized, then the deserializer tries to use the exact same type by its assembly name, and full type name.

Oops, wrong link: http://www.newtonsoft.com/json/help/html/serializetypenamehandling.htm

angularsen · 2016-10-31T21:49:04Z

UnitsNet.Serialization.JsonNet.Tests/UnitsNetJsonConverterTests.cs

+                var deserializedTestObject = JsonConvert.DeserializeObject<TestObjWithThreeIComparable>(json, jsonSerializerSettings);
+
+                Assert.That(deserializedTestObject.Value1.GetType(), Is.EqualTo(comparable1.GetType()));
+                Assert.That(((deserializedTestObject.Value1)), Is.EqualTo(comparable1));


Redundant parantheses.

angularsen · 2016-10-31T21:49:22Z

UnitsNet.Serialization.JsonNet.Tests/UnitsNetJsonConverterTests.cs

+                Assert.That(((deserializedTestObject.Value3)), Is.EqualTo(comparable3));
+            }
+
+            public static object[] TestObjectsForThreeObjectsInIComparableWithDifferentValues_ExpectAllCorrectlyDeserialized


Can be private.

angularsen · 2016-10-31T21:49:49Z

UnitsNet.Serialization.JsonNet.Tests/UnitsNetJsonConverterTests.cs

        }

        internal class TestObj
        {
            public Frequency? NullableFrequency { get; set; }
            public Frequency NonNullableFrequency { get; set; }
        }
+
+        internal class TestObjWithValueAndUnit : IComparable


All these types can be private, can't they?

That's true. Followed the existing class, will change all of them.

angularsen · 2016-10-31T21:51:27Z

UnitsNet.Serialization.JsonNet.Tests/UnitsNetJsonConverterTests.cs

+                };
+                JsonSerializerSettings jsonSerializerSettings = CreateJsonSerializerSettigns();
+
+                string json = JsonConvert.SerializeObject(testObjWithIComparable,jsonSerializerSettings);


Add space between params.

eriove · 2016-11-01T09:39:15Z

Fixed the typos and formatting. Kept the TestCaseSource. Let's discuss further.

angularsen · 2016-11-01T22:15:40Z

Will try to get to this tomorrow.

On Tue, Nov 1, 2016, 10:39 Erik Ovegård notifications@github.com wrote:

Fixed the typos and formatting. Kept the TestCaseSource. Let's discuss
further.

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#200 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAwFaDjRxBAGftPYw-6rla7RXbjIXjbvks5q5wjEgaJpZM4KjW2r
.

eriove · 2016-11-02T08:59:40Z

I removed the TestCaseSource and just kept one test that tests with TestObjWithThreeIComparable.

angularsen

Looks good to me. I think this is ready for a new alpha nuget. Serialization is still kind of experimental, represented by the prerelease nuget.

angularsen · 2016-11-02T14:11:55Z

Nuget 1.0.0-alpha5 is now out.

Modified serilization to support units saved as IComparable

ded0ebe

angularsen requested changes Oct 30, 2016

View reviewed changes

Erik Ovegard added 2 commits October 31, 2016 16:30

Made changes based on code review by @anjdreas

3653228

Added missing IComparable

c56f4a4

angularsen requested changes Oct 31, 2016

View reviewed changes

Changes based on code review by @anjdreas

5a165d7

Removed TestCaseSource

5bff42e

angularsen approved these changes Nov 2, 2016

View reviewed changes

angularsen merged commit 52c8dba into angularsen:master Nov 2, 2016

eriove deleted the feature/SerilizationOfIComparable branch November 2, 2016 14:25

angularsen mentioned this pull request Jan 12, 2020

Rework of UnitsNetJsonConverter #732

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modified serilization to support units saved as IComparable #200

Modified serilization to support units saved as IComparable #200

eriove commented Oct 28, 2016

angularsen left a comment •

edited

Loading

angularsen Oct 30, 2016

eriove Oct 31, 2016

eriove commented Oct 31, 2016 •

edited

Loading

angularsen left a comment

angularsen Oct 31, 2016

angularsen Oct 31, 2016

eriove Nov 1, 2016 •

edited

Loading

angularsen Nov 2, 2016 •

edited

Loading

angularsen Nov 2, 2016

angularsen Oct 31, 2016

angularsen Oct 31, 2016

angularsen Oct 31, 2016

eriove Nov 1, 2016

angularsen Oct 31, 2016

eriove commented Nov 1, 2016

angularsen commented Nov 1, 2016

eriove commented Nov 2, 2016

angularsen left a comment

angularsen commented Nov 2, 2016

Modified serilization to support units saved as IComparable #200

Modified serilization to support units saved as IComparable #200

Conversation

eriove commented Oct 28, 2016

angularsen left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriove commented Oct 31, 2016 • edited Loading

angularsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriove Nov 1, 2016 • edited Loading

Choose a reason for hiding this comment

angularsen Nov 2, 2016 • edited Loading

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

eriove commented Nov 1, 2016

angularsen commented Nov 1, 2016

eriove commented Nov 2, 2016

angularsen left a comment

Choose a reason for hiding this comment

angularsen commented Nov 2, 2016

angularsen left a comment •

edited

Loading

eriove commented Oct 31, 2016 •

edited

Loading

eriove Nov 1, 2016 •

edited

Loading

angularsen Nov 2, 2016 •

edited

Loading