Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix JSON serialization of quantities with decimal values #868

Merged
merged 1 commit into from
Dec 17, 2020

Conversation

rohahn
Copy link
Contributor

@rohahn rohahn commented Dec 15, 2020

Add IDecimalQuantity interface to expose the decimal value
Serialize decimal values as string to keep number of decimal places

@codecov
Copy link

codecov bot commented Dec 15, 2020

Codecov Report

Merging #868 (e9a8161) into master (4993a2a) will decrease coverage by 0.35%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #868      +/-   ##
==========================================
- Coverage   83.09%   82.74%   -0.36%     
==========================================
  Files         287      287              
  Lines       42987    43332     +345     
==========================================
+ Hits        35720    35853     +133     
- Misses       7267     7479     +212     
Impacted Files Coverage Δ
...Serialization.JsonNet/UnitsNetBaseJsonConverter.cs 100.00% <100.00%> (ø)
UnitsNet/GeneratedCode/Quantities/BitRate.g.cs 85.43% <100.00%> (+0.09%) ⬆️
UnitsNet/GeneratedCode/Quantities/Information.g.cs 86.09% <100.00%> (+0.09%) ⬆️
UnitsNet/GeneratedCode/Quantities/Power.g.cs 85.20% <100.00%> (+0.10%) ⬆️
UnitsNet/GeneratedCode/Quantity.g.cs 52.00% <0.00%> (-29.41%) ⬇️
UnitsNet/CustomCode/Quantity.cs 81.08% <0.00%> (-3.13%) ⬇️
UnitsNet/QuantityTypeConverter.cs 98.38% <0.00%> (-0.03%) ⬇️
UnitsNet/GeneratedCode/Quantities/Length.g.cs 94.00% <0.00%> (+0.02%) ⬆️
UnitsNet/GeneratedCode/Quantities/VolumeFlow.g.cs 90.01% <0.00%> (+0.03%) ⬆️
... and 102 more

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 4993a2a...e9a8161. Read the comment docs.

@angularsen
Copy link
Owner

Solid work on this 👏 Love the tests!

I think it is clever to use string to support decimal, but I'm concerned we are breaking backwards compatibility on the JSON schema for those expecting to read Value as a number.

How about this:

  • Keep Unit and Value exactly as before, the double representation of the number matching our common public interface for quantities.
  • If value is not double, then add two more fields
    • ValueString with textual representation of number
    • ValueType with string values like "decimal", "float", "SomeLibrary.BigInt"

This makes the schema more extensible to other types of numbers with generics, as have been discussed a number of times:
#180
#285
#666
#714

@rohahn rohahn force-pushed the decimal-serialization branch from 75665e8 to c5af7a1 Compare December 17, 2020 15:56
@rohahn
Copy link
Contributor Author

rohahn commented Dec 17, 2020

I implemented your proposal.

Add IDecimalQuantity interface to expose the decimal value
Serialize decimal values as string to keep number of decimal places
@rohahn rohahn force-pushed the decimal-serialization branch from c5af7a1 to e9a8161 Compare December 17, 2020 17:14
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 perfect to me, great work on this!

@angularsen angularsen merged commit 25b6558 into angularsen:master Dec 17, 2020
@angularsen
Copy link
Owner

Nuget is on the way out, I'd appreciate it if you gave it a spin to ensure it works as expected and comment back here! 🙇
Release JsonNet/4.3.0 · angularsen/UnitsNet

@rohahn
Copy link
Contributor Author

rohahn commented Dec 18, 2020

Why is the version number 4.3.0?

@angularsen
Copy link
Owner

JsonNet nuget is versioned separately from UnitsNet nuget, but they share major version.

angularsen added a commit that referenced this pull request Mar 19, 2021
Fixes parsing JSON for decimal quantities like Power on machines with cultures like Norwegian, where a comma is decimal separator is used.

Related to #847, #868
angularsen added a commit that referenced this pull request Mar 19, 2021
Fixes parsing JSON for decimal quantities like Power on machines with cultures like Norwegian, where a comma is decimal separator is used.

Related to #847, #868
angularsen added a commit that referenced this pull request Apr 4, 2021
Fixes parsing JSON for decimal quantities like Power on machines with cultures like Norwegian, where a comma is decimal separator is used.

Related to #847, #868
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.

2 participants