New unit: TorquePerLength and fix for array serialization & deserialization #712

dschuermans · 2019-10-17T15:09:02Z

Added a new unit: TorquePerLength
This unit is used in the structural analysis field when expressing the rotational stiffness resistance around an axis

Also added support for serialization & deserialization of an array of units in JSON

Added support for serializing/deserializing array of units

… failing on array serialization and deserialization functionality

codecov-io · 2019-10-17T15:18:34Z

Codecov Report

❗ No coverage uploaded for pull request base (master@c447451). Click here to learn what that means.
The diff coverage is 67.18%.

@@            Coverage Diff            @@
##             master     #712   +/-   ##
=========================================
  Coverage          ?   58.44%           
=========================================
  Files             ?      168           
  Lines             ?    38304           
  Branches          ?        0           
=========================================
  Hits              ?    22386           
  Misses            ?    15918           
  Partials          ?        0

Impacted Files	Coverage Δ
UnitsNet/GeneratedCode/UnitAbbreviationsCache.g.cs	`100% <ø> (ø)`
UnitsNet/GeneratedCode/UnitConverter.g.cs	`100% <100%> (ø)`
...sNet/GeneratedCode/Quantities/TorquePerLength.g.cs	`59.9% <59.9%> (ø)`
UnitsNet/GeneratedCode/Quantity.g.cs	`81.48% <80%> (ø)`
...Net.Serialization.JsonNet/UnitsNetJsonConverter.cs	`89.28% <98.07%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c447451...92fc6dd. Read the comment docs.

dschuermans · 2019-10-21T07:51:47Z

@angularsen ping
If you don't want to merge this, is there any way i can create a pre-release package and host it on our own nuget?

dschuermans · 2019-10-21T10:34:48Z

Okay, I added the TorquePerLength on a copy from old/v3, bumped minor & suffix (3.112.0-2) and uploaded the nuget package to our nuget feed.
So i'm all set for now 👍

angularsen · 2019-10-21T12:39:33Z

I'm unavailable all this week but glad you found a way.

…

On Mon, Oct 21, 2019, 12:34 Dirk Schuermans ***@***.***> wrote: Okay, I added the TorquePerLength on a copy from old/v3, bumped minor & suffix (3.112.0-2) and uploaded the nuget package to our nuget feed. So i'm all set for now 👍 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#712?email_source=notifications&email_token=AAGAK2EV4HVR5LE2ADESBOTQPWAURA5CNFSM4JB3CXXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBZ3UNI#issuecomment-544455221>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGAK2AQAML6B25DIW5MBH3QPWAURANCNFSM4JB3CXXA> .

angularsen

Looks good. Good job fixing the compatibility tests 👍
For next time, it would be better to create two separate pull requests for adding a quantity and fixing JSON serialization. I'll look at addressing the syntax suggestions after merging this PR.

angularsen · 2019-10-29T17:03:10Z

UnitsNet.Serialization.JsonNet/UnitsNetJsonConverter.cs

@@ -41,9 +42,36 @@ public class UnitsNetJsonConverter : JsonConverter

            object obj = TryDeserializeIComparable(reader, serializer);
            // A null System.Nullable value or a comparable type was deserialized so return this
-            if (!(obj is ValueUnit vu))
+            if (!(obj is ValueUnit) && !(obj is Array))


Minor, but this could benefit from being rewritten to if (obj is ValueUnit valueUnit) {} else if (obj is Array array) { } instead.

Now that I think about it, "obj is Array" might be cutting it a bit short too?
What if it's an array of strings? or any other type?
Not entirely sure if we can even end up in that scenario though...

is Array should cover string[] I believe. It's the base type all arrays implement if I understand it right.

Yea, but shouldn't we only be expecting an array of Units there?
When it's an array of e.g. strings, we should return that array as is and not try to parse them as units?

As per the comment there: '// A null System.Nullable value or a comparable type was deserialized so return this'
I'd imagine it applies to a "array of system.nullable values or an array of comparable types"

Just thinking out loud here :)

Good questions, I don't really recall how this all works anymore and I didn't write the implementation. I would have to read some more unit tests and dig a bit more into how this is actually used.

For instance, does this JSON deserialize properly or does it trip up on the other stuff?

{ "somestring": "foo", "somenumber": 5, "some_quantity": { "Value": 5, "Unit": "LengthUnit.Meter" }, "some_quantity_array": [ { "Value": 5, "Unit": "LengthUnit.Meter" } ], "some_other_array": [ 5, "abc", ["def" ] ] }

angularsen · 2019-10-29T17:04:32Z

UnitsNet.Serialization.JsonNet/UnitsNetJsonConverter.cs

+
+                List<object> results = new List<object>();
+
+                foreach (var value in values)


This foreach can be changed to LINQ.

True, i copied it from my initial attempt and it was laid out like this for easier debugging while developing.

angularsen · 2019-10-29T17:08:16Z

UnitsNet.Serialization.JsonNet/UnitsNetJsonConverter.cs

+                }
+
+                Type unitType = objectType.GetElementType();
+                Array typedArray = Array.CreateInstance(unitType, results.Count);


Is this all actually necessary? I think if you replace the foreach and this below with a LINQ like this, I believe it should work:

var results = values.Select(value => ParseValueUnit(value as ValueUnit)).ToArray();

I think you'll run into runtime exceptions, saying it can't convert object to array or whatever...
This is how I've always converted an "object" into an type specific array (eg string[] or double[] or whatever)

You were absolutely right, it wasn't quite as simple as I first imagined. I did make some simplifications in #715 if you want to take a look.

Nice, like how you handled it there :)

angularsen · 2019-10-29T17:20:01Z

UnitsNet.Tests/CustomCode/TorquePerLengthTests.cs

+        protected override double PoundForceInchesPerFootInOneNewtonMeterPerMeter => 2.69770722235;
+        protected override double TonneForceCentimetersPerMeterInOneNewtonMeterPerMeter => 1.01971621298e-2;
+        protected override double TonneForceMetersPerMeterInOneNewtonMeterPerMeter => 1.01971621298e-4;
+        protected override double TonneForceMillimetersPerMeterInOneNewtonMeterPerMeter => 1.01971621298e-1;


Verified these.

Found during PR review #712. Addressed separately.

dschuermans · 2019-10-30T11:21:07Z

Looks good. Good job fixing the compatibility tests 👍
For next time, it would be better to create two separate pull requests for adding a quantity and fixing JSON serialization. I'll look at addressing the syntax suggestions after merging this PR.

Cheers.

Yea, having to do the merge from v3 to v4 it was easier to also incorporate my fixes for the JSON stuff...
Sorry about that one!

angularsen · 2019-11-22T00:45:18Z

I seem to have forgot to release the nuget. Here it is.

Release UnitsNet/4.40.0 · angularsen/UnitsNet

Dirk Schuermans and others added 6 commits December 3, 2018 15:01

Added support for serializing array of units

dcf2c24

Added support for deserialization

356583b

Merge pull request #1 from dschuermans/bug/array-serialization

dbf8da0

Added support for serializing/deserializing array of units

Merged changes to json converter

7a77ae4

Fixed build errors in JsonConverter

b8622a4

Added TorquePerLength unit and fixed issue with compatibility project…

92fc6dd

… failing on array serialization and deserialization functionality

angularsen approved these changes Oct 29, 2019

View reviewed changes

angularsen merged commit 3694c5f into angularsen:master Oct 29, 2019

angularsen added a commit that referenced this pull request Oct 29, 2019

Code cleanup in JSON code

5984479

Found during PR review #712. Addressed separately.

angularsen mentioned this pull request Oct 29, 2019

Code cleanup in JSON code #715

Merged

angularsen added a commit that referenced this pull request Oct 29, 2019

Code cleanup in JSON code (#715)

a550f1b

Found during PR review #712. Addressed separately.

This was referenced Jan 7, 2020

Missing functionality in UnitsNetJsonConverter #731

Closed

Rework of UnitsNetJsonConverter #732

Closed

lipchev mentioned this pull request Sep 24, 2024

v6 Wishlist #1200

Open

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New unit: TorquePerLength and fix for array serialization & deserialization #712

New unit: TorquePerLength and fix for array serialization & deserialization #712

dschuermans commented Oct 17, 2019 •

edited

Loading

codecov-io commented Oct 17, 2019

dschuermans commented Oct 21, 2019

dschuermans commented Oct 21, 2019

angularsen commented Oct 21, 2019 via email

angularsen left a comment

angularsen Oct 29, 2019

dschuermans Oct 30, 2019

angularsen Oct 30, 2019

dschuermans Oct 30, 2019

angularsen Oct 30, 2019

angularsen Oct 29, 2019

dschuermans Oct 30, 2019

angularsen Oct 29, 2019

dschuermans Oct 30, 2019

angularsen Oct 30, 2019

dschuermans Oct 30, 2019

angularsen Oct 29, 2019

dschuermans commented Oct 30, 2019

angularsen commented Nov 22, 2019


		List<object> results = new List<object>();

		foreach (var value in values)

New unit: TorquePerLength and fix for array serialization & deserialization #712

New unit: TorquePerLength and fix for array serialization & deserialization #712

Conversation

dschuermans commented Oct 17, 2019 • edited Loading

codecov-io commented Oct 17, 2019

Codecov Report

dschuermans commented Oct 21, 2019

dschuermans commented Oct 21, 2019

angularsen commented Oct 21, 2019 via email

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

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

dschuermans commented Oct 30, 2019

angularsen commented Nov 22, 2019

dschuermans commented Oct 17, 2019 •

edited

Loading