refactor(math): update math docs & move test code to test file #19604
Conversation
WalkthroughWalkthroughThis set of changes involves the removal and refactoring of functions and methods across several files in a math-related codebase. Key modifications include the elimination of legacy testing functions and the renaming of methods for clarity and improved usability. Additionally, there are updates to comments and minor formatting adjustments. Changes
Related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yml
Files selected for processing (4)
- math/dec.go (2 hunks)
- math/dec_test.go (1 hunks)
- math/int.go (4 hunks)
- math/uint.go (5 hunks)
Additional comments: 26
math/uint.go (4)
- 24-24: The method
BigIntMutis introduced as a replacement forBigInt, with the intention of making it clear that the originalUintis mutated. This change improves clarity but ensure that all references toBigIntare updated accordingly.- 102-102: Renaming
AddtoAddUint64improves clarity by specifying that the method operates onUint64types. This is a positive change for readability and consistency.- 116-116: Similar to
AddUint64, renamingMultoMulUint64enhances clarity regarding the method's operation onUint64types. This change aligns with best practices for method naming.- 142-142: The introduction of
QuoUint64follows the pattern of making method operations explicit in their names. This consistency in naming helps in understanding the method's purpose without needing to dive into its implementation.math/int.go (2)
- 335-335: Updating the comment for
MulRawto specify that it multiplies anIntand anint64is a minor but helpful clarification. This change improves the documentation and makes the method's purpose clearer.- 416-416: The updated comment for the
Stringmethod enhances clarity by indicating its purpose is to provide a human-readable string representation of theInt. This is a good practice for improving code readability and maintainability.math/dec.go (16)
- 7-12: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The package declaration and import statements are standard and correctly formatted. It's good practice to ensure that all imported packages are used within the file to avoid unnecessary dependencies.
- 10-10: The
LegacyDecstruct is well-documented, emphasizing the importance of not usingnew(Dec)directly to avoid panics during unmarshalling. This is a good example of using comments to guide correct usage patterns.- 7-12: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [12-45]
Constants and global variables are clearly defined and documented. The use of constants for precision and related calculations enhances readability and maintainability. It's important to ensure that these values are consistent and correctly used throughout the codebase.
- 7-12: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [47-55]
The error definitions are straightforward and descriptive, aiding in debugging and error handling. Consistent naming conventions for errors (
ErrLegacy...) improve code readability.
- 7-12: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [57-64]
The
initfunction for setting precision multipliers is a good use of Go's initialization capabilities. However, it's crucial to ensure that this function does not introduce side effects that could affect the application's predictability.
- 7-12: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [66-66]
The
precisionIntfunction is a simple and effective way to reuse theprecisionReusebig.Int instance. This approach minimizes allocations and enhances performance.
- 7-12: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [68-68]
The functions
LegacyZeroDec,LegacyOneDec, andLegacySmallestDecprovide convenient ways to create common decimal values. This enhances code readability and reuse.
- 7-12: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [70-83]
The
calcPrecisionMultiplierandprecisionMultiplierfunctions are key components of the decimal precision handling. It's important to ensure that these functions are correctly implemented to avoid precision errors in arithmetic operations.
- 7-12: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [85-85]
The
LegacyNewDecand related functions for creating newLegacyDecinstances are well-structured and provide clear contracts through comments. Ensuring that these contracts are adhered to in all usage contexts is crucial for maintaining correctness.
- 7-12: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [87-87]
The
LegacyNewDecFromStrfunction is a critical part of the API, allowing for the creation ofLegacyDecinstances from strings. It's important to thoroughly test this function, especially given its complexity and the various edge cases it needs to handle.
- 7-12: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [89-89]
The arithmetic and comparison functions (
Add,Sub,Mul, etc.) are central to the functionality of theLegacyDectype. It's essential to ensure that these functions are correctly implemented and optimized for performance, especially considering the potential for high-frequency usage in financial calculations.
- 7-12: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [91-91]
The
ApproxRootandPowerfunctions, which perform more complex mathematical operations, are well-documented and appear to be carefully implemented. However, given the complexity of these operations, additional tests to cover edge cases and ensure accuracy would be beneficial.
- 7-12: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [93-93]
The
String,Format, and related formatting functions are crucial for displayingLegacyDecvalues in a human-readable form. Ensuring that these functions handle all edge cases correctly, especially regarding rounding and precision, is important for user experience.
- 7-12: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [95-95]
The
chopPrecisionAndRoundand related functions are essential for managing decimal precision. These functions must be carefully reviewed to ensure that they correctly implement the intended rounding and truncation behavior without introducing errors.
- 7-12: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [97-97]
The serialization and marshalling functions (
MarshalJSON,UnmarshalJSON, etc.) are correctly implemented, following best practices for handling nil values and ensuring compatibility with external systems. It's important to maintain comprehensive tests for these functions to prevent issues with data interchange.
- 7-12: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [99-99]
The helper functions (
LegacyDecsEqual,LegacyMinDec,LegacyMaxDec, etc.) provide useful utilities for working withLegacyDecinstances. These functions enhance the API's usability and should be kept up to date with any changes to theLegacyDectype's behavior.math/dec_test.go (4)
- 27-37: The introduction of
legacyDecEqandlegacyDecApproxEqhelper functions is a good practice for DRY (Don't Repeat Yourself) and enhances the readability of test failure messages by providing more descriptive output. However, ensure that these functions are consistently used across all relevant tests to maintain uniformity in test failure reporting.- 45-59: The refactoring to use
legacyDecApproxEqinTestDecApproxEqfor testing approximate equality is correctly implemented. This change improves the clarity of test failure messages, aiding in quicker debugging. It's important to ensure that the tolerance values chosen for these tests are appropriate for the precision required by the application logic.- 24-62: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The test suite is comprehensive, covering a wide range of functionalities provided by the
mathpackage. It's well-structured, making it easy to identify and add new test cases as needed. Consider adding tests for any new functionalities introduced in themathpackage that are not yet covered by this suite to ensure thorough coverage.
- 24-62: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The tests for
TestFormatDecandTestFormatDecNonDigitsare well-implemented, providing coverage for the formatting of decimal strings, including error cases. It's good practice to also include tests for edge cases, such as extremely large or small numbers, to ensure the formatting logic behaves as expected under all conditions.
| suite.Run(t, new(decimalTestSuite)) | ||
| } | ||
|
|
||
| // LegacyDecEq intended to be used with require/assert: require.True(DecEq(...)) | ||
| func legacyDecEq(t *testing.T, exp, got math.LegacyDec) (*testing.T, bool, string, string, string) { | ||
| t.Helper() | ||
| return t, exp.Equal(got), "expected:\t%v\ngot:\t\t%v", exp.String(), got.String() | ||
| } | ||
|
|
||
| func legacyDecApproxEq(t *testing.T, d1, d2, tol math.LegacyDec) (*testing.T, bool, string, string, string) { | ||
| t.Helper() | ||
| diff := d1.Sub(d2).Abs() | ||
| return t, diff.LTE(tol), "expected |d1 - d2| <:\t%v\ngot |d1 - d2| = \t\t%v", tol.String(), diff.String() | ||
| } | ||
|
|
||
| func TestDecApproxEq(t *testing.T) { | ||
| // d1 = 0.55, d2 = 0.6, tol = 0.1 | ||
| d1 := math.LegacyNewDecWithPrec(55, 2) | ||
| d2 := math.LegacyNewDecWithPrec(6, 1) | ||
| tol := math.LegacyNewDecWithPrec(1, 1) | ||
|
|
||
| require.True(math.LegacyDecApproxEq(t, d1, d2, tol)) | ||
| require.True(legacyDecApproxEq(t, d1, d2, tol)) | ||
|
|
||
| // d1 = 0.55, d2 = 0.6, tol = 1E-5 | ||
| d1 = math.LegacyNewDecWithPrec(55, 2) | ||
| d2 = math.LegacyNewDecWithPrec(6, 1) | ||
| tol = math.LegacyNewDecWithPrec(1, 5) | ||
|
|
||
| require.False(math.LegacyDecApproxEq(t, d1, d2, tol)) | ||
| require.False(legacyDecApproxEq(t, d1, d2, tol)) | ||
|
|
||
| // d1 = 0.6, d2 = 0.61, tol = 0.01 | ||
| d1 = math.LegacyNewDecWithPrec(6, 1) | ||
| d2 = math.LegacyNewDecWithPrec(61, 2) | ||
| tol = math.LegacyNewDecWithPrec(1, 2) | ||
|
|
||
| require.True(math.LegacyDecApproxEq(t, d1, d2, tol)) | ||
| require.True(legacyDecApproxEq(t, d1, d2, tol)) | ||
| } | ||
|
|
||
| // create a decimal from a decimal string (ex. "1234.5678") |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
For benchmark tests, it's beneficial to include a brief comment explaining what each benchmark aims to measure. This helps maintainers and contributors understand the purpose of the benchmarks and can guide future optimizations.
+ // BenchmarkMarshalTo measures the performance of the MarshalTo function.
func BenchmarkMarshalTo(b *testing.B) {
...
}
+ // BenchmarkLegacyQuoMut measures the performance of the QuoMut function on LegacyDec.
func BenchmarkLegacyQuoMut(b *testing.B) {
...
}
| @@ -333,7 +332,7 @@ func (i Int) Mul(i2 Int) (res Int) { | |||
| return x | |||
| } | |||
|
|
|||
| // MulRaw multipies Int and int64 | |||
| // MulRaw multiples Int and int64 | |||
There was a problem hiding this comment.
| // MulRaw multiples Int and int64 | |
| // MulRaw multiplies Int and int64 |
| @@ -924,18 +923,6 @@ func LegacyMaxDec(d1, d2 LegacyDec) LegacyDec { | |||
| return d1 | |||
| } | |||
|
|
|||
| // LegacyDecEq intended to be used with require/assert: require.True(DecEq(...)) | |||
| func LegacyDecEq(t *testing.T, exp, got LegacyDec) (*testing.T, bool, string, string, string) { | |||
There was a problem hiding this comment.
Removing this is api breaking.
| @@ -414,7 +413,7 @@ func MaxInt(i, i2 Int) Int { | |||
| return Int{max(i.BigInt(), i2.BigInt())} | |||
| } | |||
|
|
|||
| // Human readable string | |||
| // String Human readable string | |||
There was a problem hiding this comment.
u mean String returns human readable string
Submitted twice, update math docs & move test code to test file
Summary by CodeRabbit