Extendable json serialisation #739
Conversation
…if only because GenericTryPass is never constructed).
Codecov Report
@@ Coverage Diff @@
## master #739 +/- ##
==========================================
+ Coverage 58.58% 61.94% +3.35%
==========================================
Files 171 173 +2
Lines 38448 38547 +99
==========================================
+ Hits 22525 23878 +1353
+ Misses 15923 14669 -1254
Continue to review full report at Codecov.
|
|
Hi and thanks for sharing your approach with us. I like the idea of better supporting third-party quantities, but some comments below on how we can maybe fit it better into the library. 1.
|
|
Thanks for your feedback.
I agree that the static functions are a weak architecture – they came out of a desire to minimise the refactoring to the remainder of the code necessary to implement the JSON serialisation. The same applies to the support for Quantity.From – hoping to have the custom quantities perform like standard ones in as many contexts as possible.
For what it is worth, the biggest blocker I hit on this was the QuantityType enum – as I was trying to avoid changing the code generation and I don’t believe it is possible to add values to an enum at runtime. At the moment where these are relevant I am returning QuantityType.Undefined, but I find this unsatisfactory / potentially confusing.
The upside of the static approach is that I could register a custom type in my project just by calling Quantity.AddUnit once at startup – and then everything subsequent doesn’t have to know or care about the custom type. It does however make unit tests difficult. It would also make a horrible mess of any use cases where code wants to redefine a unit on the fly; you would hope no one would want to do this, but people are very creative.
I think I may struggle to get sufficiently on top of the codebase to wholesale strip out a factory class in the time I have available. I will have a go and see how that looks – comparing it to just having a factory class that knows about the custom types and leaving a broader refactor for later work.
Cheers,
David
From: Andreas Gullberg Larsen <notifications@github.com>
Sent: 21 January 2020 20:57
To: angularsen/UnitsNet <UnitsNet@noreply.github.com>
Cc: David Baker <david_baker@iinet.net.au>; Author <author@noreply.github.com>
Subject: Re: [angularsen/UnitsNet] Extendable json serialisation (#739)
Hi and thanks for sharing your approach with us. I like the idea of better supporting third-party quantities, but some comments below on how we can maybe fit it better into the library.
1. Quantity.AddUnit() and Quantity.ExternalQuantities cannot be static.
As an example, in your tests you are adding a unit mapping in one test, and that mapping lives on to the next tests that run. It is prone to cause bugs like that.
From your test case
https://github.com/angularsen/UnitsNet/pull/739/files#diff-4593c2e3fa9c32725a300f36f3951db4R47-R53
it seems you want this in order to be able to do IQuantity q = Quantity.From(1, myCustomUnit).
I propose we do the same as we did for UnitConverter in https://github.com/angularsen/UnitsNet/blob/25ff826de0d7c2cc58bfa573362295fd05470626/UnitsNet/UnitConverter.cs#L51-L64.
https://github.com/angularsen/UnitsNet/blob/25ff826de0d7c2cc58bfa573362295fd05470626/UnitsNet.Tests/UnitConverterTest.cs#L95-L111
Use singleton pattern to make it testable, yet easy to manipulate and use via the Default property.
Maybe something like this?
public class QuantityFactory
{
public static QuantityFactory Default { get; } = new QuantityFactory();
public void SetCreateFunction<TQuantity>(Enum unit, Func<QuantityValue, TQuantity>)
where TQuantity : IQuantity
{ /* ...magic here... */ }
public TQuantity Create<TQuantity>(QuantityValue value, Enum unit)
where TQuantity : IQuantity
{ /* ...magic here... */ }
}
Note that QuantityValue is a wrapper type that implicitly casts decimal or double values. We introduced this to reduce the binary size, because 1000+ units with twice the method count actually became significantly larger.
With this, I don't think we need Quantity.From() anymore, but we could make that work too if we wanted to.
2. Take QuantityFactory as parameter in the JsonConverter
Now that we have a configurable and testable way to construct quantities we can pass in an QuantityFactory instance to the converter. QuantityFactory.Default would then be the default instance passed unless otherwise specified.
Thoughts?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#739?email_source=notifications&email_token=AEIH6CONJ7V5K7SYTQYXGLLQ65OQVA5CNFSM4KJPQGE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJRHJAY#issuecomment-576877699> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEIH6CJOYJHASAUMXXR2XSDQ65OQVANCNFSM4KJPQGEQ> . <https://github.com/notifications/beacon/AEIH6CNIJ4LXJAZQLUKXTMDQ65OQVA5CNFSM4KJPQGE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJRHJAY.gif>
|
|
OK - much wider changes to the code now, but a nicer solution without the static list. I didn't quite follow the suggestion on implementation approach, preferring instead to mostly change leave the existing signatures alone. I moved the object generation functions into QuantityInfo - where they can be created by reflection when the type is built. I then created a dictionary of types in a QuantityFactory type as suggested and moved the quantity generation logic from both Quantity.cs and the JsonSerialiser class into this. As a result of this there is a greatly reduced need for Quantity.g.cs - it is now only used in generating the Infos list. I expected changing to reflection approaches to have a bad impact on performance, but I'm not actually seeing that. My profiling seems to suggest this approach is no slower, though would be worth verifying that. |
|
Well, i'm not a big fan of adding a QuantityFactory parameter to the ctors of the JsonConverters because it adds an explicit dependency on the QuantityFactor and it might become troublesome when used in a dependency injection setup If i understand correctly, the problem you are running into is that you are unable to deserialize a json If i were to go this approach, I would expect some kind of service / factory like your QuantityFactory to exist in UnitsNet, which already knows about all the available units shipped with UnitsNet. It is then up to your application, during its bootstrapping phase to tell UnitsNet that you have defined some additional units in your assemblies and where UnitsNet can find them and how they should be constructed. Then the UnitsNetBaseJsonConverter can simply access the QuantityFactory directly (without it having to be provided through the ctor) and attempt to construct any type from the data it extracted from the JSON string. Psudo example: These are my 2 cents 😄 |
There was a problem hiding this comment.
I've made quite a bit of modifications to the PR just to move this along faster. Please take a look at my commits and let me know what you think.
The main changes beyond a bit of cleanup and renames, are
- Remove reflection code
- Replace with
CustomQuantityOptionsthat custom quantities should implement and pass intoQuantityInfoctor. I made it optional for now, so I didn't have to fix all the generated code.
One realization I had was that we should probably move Quantity.TryFrom generated code into QuantityInfo.TryFrom, so that the same design is used for both generated quantities and custom quantities - similar to how we do for mapping built-in and third-party conversion functions between units.
I have a feeling we should add some more tests, there are a bunch of new public members in this PR and not so many tests added even though they are probably called indirectly. I generally like tests on all public members, but I don't have more time today. If you could take a look at that, it would be great.
UnitsNet.Serialization.JsonNet.Tests/UnitsNetJsonConverterTests.cs
Outdated
Show resolved
Hide resolved
|
@dschuermans I'm short on time, but I think maybe we are getting best of both worlds now?
I think this is what we are getting with this design? You can now do IQuantity oneMeter = QuantityFactory.Default.From(1, LengthUnit.Meter);
QuantityFactory.Default.AddUnit(HowMuch.Info);
IQuantity oneSillyTon = QuantityFactory.Default.Parse(typeof(HowMuch), "1 sillyton");Let me know if I got anything wrong, I kinda rushed a bit to get through this PR tonight. |
I agree with point 1. About 2: |
|
I see your concern, but I really don't think it's all that bad. We could very well let At least this way, we have the opportunity to fully control the configuration at runtime - but more importantly we can write tests that don't affect other tests that happen to use the same static values. Global static values is a painful design in my opinion, singletons offers a good compromise of being convenient yet testable. |
|
I think the pattern of using a QuantityFactory.Default in almost all cases here, but having the ability to construct alternatives, is the right balance here. I struggle to think of any use cases outside of unit testing where you would want to have different quantity factories - but I can't think how you'd do unit testing well without it. I don't really agree with the philosophy of moving away from reflection in these cases - though I suspect some of the reflection can be done more cleanly. It feels to me like there is a lot of non-obvious boilerplate code required to make a custom instance of IQuantity work well. I was working to a philosophy of minimising the amount of code needed in IQuantity - even if that made the QuantityInfo class more complex. The AddUnit call in QuantityFactory is a case in point - I see virtue in being able to call it from external code using type names and leaving the AddUnit call to do the heavy lifting of creating an instance to get the QuantityInfo data - rather than requiring anyone calling AddUnit to create an instance. Supporting both might be an option? |
|
Great input, I'll try to take another look tonight. The design for
supporting third-party quantities is absolutely still up in the air and
kind of introducing itself organically now to solve specific needs. It is
not a holistic design yet.
I didn't quite get a handle on why reflection made the integration easier,
but I'll give another read later. I think we should maybe take a step back
and consider exactly what should be in IQuantity vs QuantityInfo vs
Quantity/QuantityFactory and how to best support third-party quantities
that way.
If we can avoid reflection, I'd prefer so, but if it adds real value then
yes, we'll do that.
|
|
@dschuermans A follow up:
I agree, and I believe that is exactly what we achieve by having the default ctor pick the singleton and the more specific ctor take an explicit factory object, which is entirely optional to use. The converter does not have to decide anything on that regard. The whole reason for adding this factory was to support third-party quantities. The reason for making it possible to inject the factory into the converter was to make it testable. We already had support for converting between third-party quantities/units, now we also have support for deserializing and serializing third-party quantities.
Maybe, but I generally try to avoid inheritance if I can. It tends to create more complexity than composition and injecting dependencies. |
|
It's all good. Are you first going to merge this or my rework? |
|
@dschuermans I'm not sure, depends on which one feels more ready to merge I guess. I have yet to look at yours, but I plan to in a few moments if I get enough time. |
I agree, but please mind that I'm all for reducing the boilerplate needed to add support for new quantities, but I really want to avoid reflection to achieve that if we can. It's just too prone to bugs in the future when we refactor and change method signatures and if we missed a test case. A holistic approach to third-party quantitiesOkay, I just had to ramble down some thoughts to clear my own mind a bit. It's out of scope for this PR, but I think we should at least give thought to how we can best solve support for third-party quantities as a whole. These are the things I can think of that we need to solve (or have solved):
Some challenges
Idea - Introduce a facade type to configure Units.NETTo make the API more consistent for built-in types vs third-party types and to gather everything related to global configuration of Units.NET, I think we should consider adding a facade configuration type.
|
|
For this PR though, to move this forward, I think we are not too far off. I still favor configuring convert/parse/create functions like in the proposal above, instead of reflection. To reduce boilerplate for this PR, we can start by moving the create/parse functions from Thoughts? |
|
KISS :wink |
|
That is a fair point, it is easy to get overboard on this stuff. Supporting third-party stuff is not trivial though, requires some thinking to fit it into all the existing code and make it intuitive to use. We also need to consider how many will actually use Units.NET for third party quantities and whether it is really worth too big changes. |
|
I definitely agree that there is a risk of over engineering here in the pursuit of a low yield use case. The units in Units.NET are quite comprehensive already, so the places where something new would be required are quite limited. The use case in Cumulus MX is almost embarrassingly trivial for example - simply trying to be able to work with a single IStatistics implementation rather than needing a special case for IStatistics. The thinking in @angularsen's notes on add-in support are all very good. A few observations:
There might be a viable middle way of avoiding reflection in the base code, but having a (suggested?) CustomQuantityBase class that takes a minimalist interface and uses it to reflect the other details necessary to create a full IQuantity. |
|
@dandm1 Could you please elaborate a bit on your usecase? It's very useful to have a concrete example to discuss around. What is This PR first started off as something that seemed easy enough and valuable to add. It then triggered some deeper thoughts about third-party quantity support and the scope suddenly increased to solve it with a better overall design. Then it raised the question about whether many will actually use this. I feel I need to mature this thinking a bit before we can continue this PR, so we don't just add something that doesn't solve many real needs and complicates the Units.NET design further. |
|
In the CumulusMX context we are using various IQuantities to capture the observations from a weather station. There is an IStatistics<> class that calculates statistics based on those observations - maximums, minimums, averages, rates of change, etc over daily, monthly, annual and rolling horizons. We/I then save the results by serialising the whole structure as JSON. So far so good, except that some of the observable quantities are unitless - doubles and integers - so we ended up with two different implementations of IStatistics : And a whole lot of branching based on the generic type. This became too much duplication, so in the name of simplicity I implemented As I said, an almost embarrassingly simple use case - and certainly there are other approaches:
|
|
Thanks, will get back to you when I find time. |
|
Ok, a couple of thoughts.
https://dotnetfiddle.net/K11USx var masses = new IQuantity[]{ Mass.FromKilograms(50), Mass.FromKilograms(100) };
var lengths = new IQuantity[]{ Length.FromCentimeters(50), Length.FromCentimeters(100) };
IQuantity avgMass = masses.Average(MassUnit.Gram);
IQuantity sumLength = lengths.Sum(LengthUnit.Meter);
IQuantity minLength = lengths.Min(LengthUnit.Meter);
IQuantity maxLength = lengths.Max(LengthUnit.Meter);
Console.WriteLine("Avg: {0}", avgMass); // Avg: 75,000 g
Console.WriteLine("Sum: {0}", sumLength); // Sum: 1.5 m
Console.WriteLine("Min: {0}", minLength); // Min: 0.5 m
Console.WriteLine("Max: {0}", maxLength); // Max: 1 m
|
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
4 participants
Extensions to the Quantity class to allow the registration of classes implementing IQuantity from outside the UnitsNet library, and to support JSON serialisation and Quantity.From with this classes.
I am a developer on v4 of the CumulusMX weather station software. The weather measurements on this are a combination of measures that have units and simple scalars. To simplify the code, we have decided to implement the scalars as a custom IQuantity - but were finding that this prevented us effectively serialising and deserialising the state. By registering the custom type using the new functionality in this pull request we should resolve that.