Add Decimal2 type for precise 2-decimal-place values#1283
Conversation
Replaces f64 for monetary fields (raw_price, renew_raw_price, sale_cost) in domain suggestions to avoid floating-point precision issues.
XCFramework BuildThis PR's XCFramework is available for testing. Add to your .package(url: "https://github.com/automattic/wordpress-rs", branch: "pr-build/1283")Built from 7ea2b13 |
| /// A decimal number with at most 2 fractional digits, stored as an integer | ||
| /// count of hundredths to avoid floating-point precision issues. | ||
| /// | ||
| /// Deserialization rejects values with more than 2 decimal places rather |
There was a problem hiding this comment.
This is probably the right answer, though I wonder if we'll eventually hit a case where an API fails to round for us. I guess in that case we just fix the API.
There was a problem hiding this comment.
We can add a Decimal3 or Decimal4 if that helps? I think Decimal2 is still useful, so I'd suggest keeping it for cases when we know it's only 2 decimals.
I guess one option was to turn it into a String and not let clients do math on it, but then you might have people parsing it, so... I think a solution like this is safer.
There was a problem hiding this comment.
My thinking was that if it's Decimal2 we know how how many significant digits the developer would like it rounded to, so we could coerce it and only implement Decimal4 if we decide we have a type that needs to be that specific.
There was a problem hiding this comment.
We started this work for money related calculations and the potential of it being wrong when we used floats. Using any kind of rounding in this type feels quite wrong to me. Am I missing something?
There was a problem hiding this comment.
I'm ok to leave it for now. We can revisit if in the future some API response expresses a dollar amount as 1.635 or something.
jkmassel
left a comment
jkmassel
left a comment
There was a problem hiding this comment.
This generally looks good in its current form, but I had a few questions.
|
|
||
| fn visit_i64<E: de::Error>(self, v: i64) -> Result<Self::Value, E> { | ||
| Ok(Decimal2 { | ||
| hundredths: v * 100, |
There was a problem hiding this comment.
Do we want to use checked_mul for this and error out if the value is unreasonably large? Seems like overkill, but it'd never be incorrect?
|
|
||
| struct Decimal2Visitor; | ||
|
|
||
| impl de::Visitor<'_> for Decimal2Visitor { |
There was a problem hiding this comment.
Should we bother implementing visit_str for values like "19.99"?
| #[case(1100, "1100")] | ||
| #[case(863, "8.63")] | ||
| #[case(0, "0")] | ||
| #[case(1800, "1800")] |
There was a problem hiding this comment.
| #[case(1100, "1100")] | |
| #[case(863, "8.63")] | |
| #[case(0, "0")] | |
| #[case(1800, "1800")] | |
| #[case(1100, "11.00")] | |
| #[case(863, "8.63")] | |
| #[case(0, "0")] | |
| #[case(1800, "18.00")] |
nit – there's a bit of inconsistency here
| /// A decimal number with at most 2 fractional digits, stored as an integer | ||
| /// count of hundredths to avoid floating-point precision issues. | ||
| /// | ||
| /// Deserialization rejects values with more than 2 decimal places rather |
There was a problem hiding this comment.
My thinking was that if it's Decimal2 we know how how many significant digits the developer would like it rounded to, so we could coerce it and only implement Decimal4 if we decide we have a type that needs to be that specific.
3 participants
Replaces f64 for monetary fields (
raw_price,renew_raw_price,sale_cost) in domain suggestions to avoid floating-point precision issues.