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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃挜Remove decimal support #1359

Merged
merged 11 commits into from
Feb 23, 2024

Conversation

Muximize
Copy link
Contributor

@Muximize Muximize commented Jan 27, 2024

Changes

  • Remove QuantityValue, replaced with double
  • Remove TValueType from interfaces
    • Remove IQuantity<TUnitType, out TValueType>
    • Remove IValueQuantity<out TValueType>
    • Change IQuantity<TSelf, TUnitType, out TValueType> to IQuantity<TSelf, TUnitType>
    • Change IArithmeticQuantity<TSelf, TUnitType, TValueType> to IArithmeticQuantity<TSelf, TUnitType>

Changes to UnitsNet.Serialiation.JsonNet

  • Deserializing previously serialized JSON for decimal quantities Information, BitRate and Power still work, but it now reads just double Value property and ignores string ValueString and string ValueType properties. This may lose precision compared to preserving the full decimal value, but decimal is no longer supported in v6.

Background

In #1195 @angularsen says:

If we change all 3 quantities to double, we have the potential to clean up a LOT of QuantityValue complexity.

This made me wonder how deep that complexity goes so I decided to experiment, and this is the result.

I must say some of these changes make me a bit sad. A lot of work and some very clever thinking went into supporting multiple numerical types, and I want to acknowledge that. 馃檱

Also, I took it as far as possible but that might not be the best outcome, for example we might want to keep deserialization support. This just demonstrates a possible direction we could go in.

@angularsen angularsen marked this pull request as ready for review February 18, 2024 19:54
@angularsen
Copy link
Owner

I synced this with release/v6 branch after merging #1195 #1353

Will do a review of this next.

@angularsen angularsen self-assigned this Feb 18, 2024
@Muximize
Copy link
Contributor Author

I rebased this to clean up the commit history, which should aid in reviewing.

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 to me!

As you said, it's a bit sad to see a lot of thought and work being thrown out, but I still do think reducing this complexity is worth it until we find a better way to support multiple numeric types in a more holistic way.

I made a few minor improvements and pushed it.

};
}

if (valueType.Type != JTokenType.String)
Copy link
Owner

Choose a reason for hiding this comment

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

I was contemplating whether we should keep the JSON serialization stuff for backwards compatibility, but I realized the JSON already was backwards compatible by storing both double and decimal (string) values. So by now only reading the double value, it should still work as before except for the change in precision that would happen even if we kept reading the decimal string value and converted that to double.

@angularsen angularsen added this to the vNext milestone Feb 23, 2024
@angularsen angularsen changed the title Remove decimal support 馃挜`Remove decimal support Feb 23, 2024
@angularsen angularsen changed the title 馃挜`Remove decimal support 馃挜Remove decimal support Feb 23, 2024
@angularsen angularsen merged commit 1d644f8 into angularsen:release/v6 Feb 23, 2024
1 check was pending
@angularsen angularsen mentioned this pull request Feb 23, 2024
7 tasks
@Muximize Muximize deleted the remove-decimal-support branch February 24, 2024 16:15
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.

None yet

2 participants