Fixes Decimal's implementation of Ord #307
Conversation
Problems addressed: * Some cases did not scale the operands for comparison properly. * If both values were negative, only their magnitudes were considered. Fixes #305.
Codecov Report
@@ Coverage Diff @@
## main #307 +/- ##
==========================================
- Coverage 91.09% 91.08% -0.01%
==========================================
Files 58 58
Lines 8421 8446 +25
==========================================
+ Hits 7671 7693 +22
- Misses 750 753 +3
Continue to review full report at Codecov.
|
| // This lets us compare 80 and 80, determining that the decimals are equal. | ||
| let mut scaled_coefficient: BigUint = d1.coefficient.magnitude().to_biguint().unwrap(); | ||
| scaled_coefficient *= 10u64.pow(exponent_delta as u32); | ||
| Magnitude::BigUInt(scaled_coefficient).cmp(&d2.coefficient.magnitude()) |
There was a problem hiding this comment.
There's not really a logic change here, but I've revised the variable names to be more accurate and added comments to try and explain what's going on.
There was a problem hiding this comment.
Thanks for the clarification.
|
|
||
| #[test] | ||
| fn test_decimal_eq() { |
There was a problem hiding this comment.
I replaced the monolithic test_decimal_eq and test_decimal_ord with #[rstest]-parameterized test functions.
| let magnitude: BigUint = big_int_coefficient | ||
| .abs() | ||
| .to_biguint() | ||
| .expect("abs() would have prevented a negative number"); |
There was a problem hiding this comment.
cargo fmt seems to have replaced this expect with an unwrap. I'm unsure why that's the preference, but I'm ok with it. 🤷
Problems addressed:
considered.
Fixes #305.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.