Skip to content

Conversation

trb5016
Copy link
Contributor

@trb5016 trb5016 commented May 9, 2020

Ran into an issue where a 2D (or any mutlidimensional array) would serialize to a 1D array and therefore couldn't be deserialized back to a 2D array based on the UnitsNetJsonConverter implementation.

Years ago in Json.Net multidimensional arrays weren't supported, so the existing Units.Net implementation may have been a result of that (used to have to do nested 1D arrays). However, modern Json.Net can serialize/deserialize mutlidimensional arrays without issue.

Added serialization, deserialization, and compatibility tests for a 2D array. Combined with the existing tests prove the new mutlidimensional array approach doesn't break the 1D arrays (or anything else tested).

@codecov-io
Copy link

codecov-io commented May 9, 2020

Codecov Report

Merging #782 into master will decrease coverage by 0.05%.
The diff coverage is 7.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #782      +/-   ##
==========================================
- Coverage   63.84%   63.79%   -0.06%     
==========================================
  Files         278      279       +1     
  Lines       41507    41544      +37     
==========================================
  Hits        26501    26501              
- Misses      15006    15043      +37     
Impacted Files Coverage Δ
...n.JsonNet/Internal/MultiDimensionalArrayHelpers.cs 0.00% <0.00%> (ø)
...Net.Serialization.JsonNet/UnitsNetJsonConverter.cs 40.55% <23.07%> (-2.41%) ⬇️

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 0c8aaa0...f48deed. Read the comment docs.

Copy link
Owner

@angularsen angularsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and good job adding the tests!
Just a couple of comments and suggestions for you to consider.

@trb5016 trb5016 requested a review from angularsen May 11, 2020 04:05
@angularsen angularsen merged commit c80157a into angularsen:master May 11, 2020
@angularsen
Copy link
Owner

Looks good to me!

Nuget is on the way out. Please give it a try.

Release JsonNet/4.2.0 · angularsen/UnitsNet

@dschuermans
Copy link
Contributor

@angularsen @trb5016
Was casually scrolling through the list of completed PRs and this one caught my attention...

Out of curiosity, why was functionality added to something marked obsolete?

Length[,] value = new Length[2,2];

value[0, 0] = Length.FromMeters(1);
value[0, 1] = Length.FromMeters(2);
value[1, 0] = Length.FromMeters(3);
value[1, 1] = Length.FromMeters(4);

var jsonSerializerSettings = new JsonSerializerSettings { Formatting = Formatting.Indented };
jsonSerializerSettings.Converters.Add(new UnitsNetIQuantityJsonConverter());

string json = JsonConvert.SerializeObject(value, jsonSerializerSettings);

Length[,] obj = JsonConvert.DeserializeObject<Length[,]>(json, jsonSerializerSettings);

Works just fine...

@trb5016
Copy link
Contributor Author

trb5016 commented Oct 1, 2020

@dschuermans
I think it was a timing issue, and lack of understanding on my part of the changes.

In my project at the time, I believe I was using JsonNet v4.0.0 which did not have UnitsNetIQuantityJsonConverter. So I was still using the old UnitsNetJsonConverter.

Then not realizing UnitsNetIQuantityJsonConverter would fix the issue, I went and made my PR despite the UnitsNetJsonConverter being marked obsolete.

The real kicker is, when v4.2.0 came out with this change in it, I finally connected the dots and realized that UnitsNetJsonConverter was obsolete , so I changed my project to use UnitsNetIQuantityJsonConverter, and so I'm not even using the fix from this PR...

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