garydgregory commented on PR #1784: URL: https://github.com/apache/commons-lang/pull/1784#issuecomment-5592764470
Hi @lenamonj Thank you for the PR, the main changes look good but I think you need more tests, specifically: - Extend the regression tests for _subtraction_. The new test covers both denominator branches for addition, but its only subtraction example has coprime denominators after reduction. Add an unreduced subtraction case with shared reduced denominators, for example `2/4 - 2/12 = 1/3`, and a cancellation case such as `2/4 - 3/6 = 0/1`. - Exercise both operands being unreduced and minimum-integer normalization. Examples such as `2/4 + 2/6 = 5/6` and `Integer.MIN_VALUE/2 + 2/4 = -2147483647/2` directly protect the two new normalization steps and their signed boundary behavior. - Bonus: Clarify the existing reduced-result promise around zero. The new normalization runs after the zero shortcuts in [`addSub` (line 550)](/Users/garygregory/git/commons/commons-lang/src/main/java/org/apache/commons/lang3/math/Fraction.java:550). Consequently, `ZERO.add(2/4)` still returns `2/4`. Also, `ZERO.subtract(Integer.MIN_VALUE/2)` still throws even though its reduced result, `1073741824/1`, fits. These are pre-existing limitations, not regressions. Thank you! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
