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 deserializing decimal quantities regardless of local culture #911

Merged
merged 1 commit into from
Apr 4, 2021

Conversation

angularsen
Copy link
Owner

@angularsen angularsen commented 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. Several tests were red on my machine, but green on build agent.

Related to #847, #868

@angularsen angularsen changed the title Parse decimal from JSON with invariant culture Fix deserializing decimal quantities regardless of local culture 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 angularsen force-pushed the angularsen/fix-json-parse-decimal-culture branch from dac9477 to d360827 Compare March 19, 2021 23:53
@codecov
Copy link

codecov bot commented Mar 20, 2021

Codecov Report

Merging #911 (d360827) into master (c5485ee) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #911      +/-   ##
==========================================
- Coverage   82.70%   82.70%   -0.01%     
==========================================
  Files         291      291              
  Lines       44040    44034       -6     
==========================================
- Hits        36423    36417       -6     
  Misses       7617     7617              
Impacted Files Coverage Δ
...Serialization.JsonNet/UnitsNetBaseJsonConverter.cs 100.00% <100.00%> (ø)
UnitsNet/UnitSystem.cs 87.50% <0.00%> (-2.50%) ⬇️
UnitsNet/UnitFormatter.cs 100.00% <0.00%> (ø)

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 c5485ee...d360827. Read the comment docs.

Copy link
Collaborator

@lipchev lipchev left a comment

Choose a reason for hiding this comment

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

As per the JSON specs the separator is indeed "." (InvariantCulture).

However, given that nobody has complained so far, I would suggest that for v5 we simply replace the Value with the ExtendedValue (i.e. the string representation of the number) and remove the ExtendedValueUnit as it's CLR type could easily be determined from the Unit (once parsed, we can check for the IDecimalQuantity).

@angularsen angularsen merged commit fbd89e7 into master Apr 4, 2021
@angularsen angularsen deleted the angularsen/fix-json-parse-decimal-culture branch April 4, 2021 21:51
@angularsen
Copy link
Owner Author

Great, yes, that sounds reasonable.

As for v5, the wishlist is piling up and there are several big ideas for changes that are still up in the air; #714, #709, class vs struct.

If you or @tmilnthorp want to start nudging the v5 snowball, I'm happy to help, but work and family has been pretty much consuming all life the past 9 months and it's likely to continue.

Baby steps is probably a good idea if we are ever to get any of the breaking changes out the door.

@angularsen
Copy link
Owner Author

@lipchev forgot to mention you. ☝️

@lipchev lipchev mentioned this pull request Apr 4, 2021
@angularsen
Copy link
Owner Author

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.

3 participants