refactor!: change rounding parameters to use defaults #89
Conversation
This commit updates the rounding parameters in multiple modules to use `Option<Rounding>` with a default value if none is specified. This change simplifies method calls and improves code flexibility by allowing optional rounding strategies.
WalkthroughThe changes in this pull request include an update to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
src/constants.rs (1)
22-22: Consider documenting the default behavior.Since
RoundHalfUpis now the default rounding strategy whenNoneis provided, it would be helpful to document this behavior in the enum's documentation./// Represents three various ways to round #[derive(Clone, Copy, Debug, Default, Hash, PartialEq, Eq)] pub enum Rounding { /// Rounds down to the nearest whole number. RoundDown, - /// Rounds to the nearest whole number, rounding halfway cases away from zero. + /// Rounds to the nearest whole number, rounding halfway cases away from zero. + /// This is the default rounding strategy when none is specified. #[default] RoundHalfUp, /// Rounds up to the nearest whole number. RoundUp, }src/entities/fractions/percent.rs (1)
97-107: Add test coverage for explicit rounding cases.While the tests verify the default rounding behavior (using None), they should also cover cases where specific rounding strategies are provided via Some(Rounding).
Consider adding these test cases:
#[test] fn test_to_significant_with_explicit_rounding() { assert_eq!( Percent::new(154, 10000) .to_significant(3, Some(Rounding::RoundDown)) .unwrap(), "1.54".to_string() ); } #[test] fn test_to_fixed_with_explicit_rounding() { assert_eq!( Percent::new(154, 10000).to_fixed(2, Some(Rounding::RoundDown)), "1.54".to_string() ); }src/entities/fractions/price.rs (1)
Line range hint
155-220: Enhance test coverage for rounding scenarios.While the current tests verify the basic functionality with
Nonerounding parameter, consider adding test cases for:
- Explicit rounding strategies using
Some(Rounding)- Edge cases where different rounding strategies would produce different results
- Boundary conditions where rounding becomes significant
This would ensure the new optional rounding parameter behaves correctly across all scenarios.
Example test cases to add:
#[test] fn test_different_rounding_strategies() { let p = Price::new(TOKEN0.clone(), TOKEN1.clone(), 123456, 789); // Test different rounding modes assert_ne!( p.to_significant(2, Some(Rounding::RoundDown)).unwrap(), p.to_significant(2, Some(Rounding::RoundUp)).unwrap(), "Rounding strategies should produce different results for this case" ); // Test None vs Some default assert_eq!( p.to_significant(2, None).unwrap(), p.to_significant(2, Some(Rounding::RoundDown)).unwrap(), "None should match the default rounding strategy" ); }src/entities/fractions/fraction.rs (1)
124-132: LGTM! Consider documenting the default rounding behavior.The change to make the rounding parameter optional is well-implemented using idiomatic Rust. The use of
unwrap_or_default()ensures safe handling of the optional parameter.Consider adding a doc comment to specify which rounding strategy is used by default when
Noneis provided.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
Cargo.toml(1 hunks)src/constants.rs(1 hunks)src/entities/fractions/currency_amount.rs(2 hunks)src/entities/fractions/fraction.rs(2 hunks)src/entities/fractions/percent.rs(3 hunks)src/entities/fractions/price.rs(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🔇 Additional comments (5)
src/constants.rs (1)
16-16: LGTM! Note: This is a breaking change.
The addition of Default trait is well-aligned with the PR objectives to support optional rounding parameters. The version bump to 3.2.0 correctly reflects this breaking change.
Let's verify the version bump:
✅ Verification successful
Version bump to 3.2.0 confirmed
The version in Cargo.toml is correctly set to 3.2.0, which aligns with the breaking change introduced by adding the Default trait to the Rounding enum.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that Cargo.toml version has been bumped
grep -A 1 '^\[package\]' Cargo.toml | grep 'version = "3.2.0"'
Length of output: 64
Script:
#!/bin/bash
# Let's check the actual version in Cargo.toml
cat Cargo.toml | grep -A 5 '^\[package\]'
# Also check git diff to see version changes
git diff origin/main Cargo.toml | grep '^[+-]version'
Length of output: 583
src/entities/fractions/percent.rs (2)
27-27: LGTM! Method signature changes align with the refactoring goals.
The change from Rounding to Option<Rounding> improves API flexibility by allowing default rounding behavior when None is provided. The implementation correctly propagates the optional rounding parameter to the underlying Fraction methods.
Also applies to: 37-37
27-27: Verify consistent usage across the codebase.
Let's ensure all callers have been updated to handle the new Option parameter.
Also applies to: 37-37
✅ Verification successful
All usages are already compatible with Option
The search results show that all calls to to_significant() and to_fixed() across the codebase are already using the optional rounding parameter correctly:
- All implementations accept
Option<Rounding> - All calls either pass
NoneorSome(Rounding::...) - No direct usage of
Rounding::withoutOptionwrapping was found
The change is consistent with the existing codebase usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining direct usage of these methods with Rounding
# This should help identify any calls that haven't been updated
echo "Searching for potential calls to to_significant that might need updating..."
rg "to_significant\([^,]+,[^,]+,\s*Rounding::"
echo "Searching for potential calls to to_fixed that might need updating..."
rg "to_fixed\([^,]+,[^,]+,\s*Rounding::"
Length of output: 401
Script:
#!/bin/bash
# Let's try a different approach to find all usages of these methods
# and verify the parameter patterns
echo "Searching for all calls to to_significant..."
rg "to_significant\(" -A 2
echo -e "\nSearching for all calls to to_fixed..."
rg "to_fixed\(" -A 2
echo -e "\nLet's also check the method definitions..."
ast-grep --pattern 'fn to_significant($$$) { $$$ }'
ast-grep --pattern 'fn to_fixed($$$) { $$$ }'
Length of output: 6723
src/entities/fractions/price.rs (1)
126-126: LGTM: Clean implementation of optional rounding parameters.
The changes to make rounding parameters optional via Option<Rounding> are well-implemented and align with the PR objectives. The implementation maintains backward compatibility while adding flexibility for callers.
Also applies to: 135-135
src/entities/fractions/fraction.rs (1)
144-145: LGTM! Verify test coverage for the optional rounding parameter.
The implementation is consistent with the changes in to_significant and follows good Rust practices.
Let's verify the test coverage for these changes:
✅ Verification successful
Test coverage exists for both to_fixed and to_significant with optional rounding parameter
The codebase shows comprehensive test coverage for both methods in multiple test cases:
src/entities/fractions/currency_amount.rshas test cases for both methods withNonerounding parameter:test_to_fixed_0_decimals:amount.to_fixed(0, None)test_to_fixed_18_decimals:amount.to_fixed(9, None)test_to_significant_0_decimals:amount.to_significant(4, None)test_to_significant_18_decimals:amount.to_significant(9, None)
The test coverage adequately verifies the default rounding behavior when None is passed as the rounding parameter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for the new optional rounding parameter
# Look for test cases that exercise the None case for rounding
rg -U "test.*\{.*to_(significant|fixed).*None.*\}" .
# Look for test cases that exercise the Some case for rounding
rg -U "test.*\{.*to_(significant|fixed).*Some.*\}" .
Length of output: 109
Script:
#!/bin/bash
# Let's try a broader search for test cases related to to_fixed and to_significant
# Search for test functions containing to_fixed
rg -U "fn test.*\{.*to_fixed.*\}" -A 10 .
# Search for test functions containing to_significant
rg -U "fn test.*\{.*to_significant.*\}" -A 10 .
# Also search for any usage of Rounding in tests
rg "Rounding" --type rust "tests/"
Length of output: 173
Script:
#!/bin/bash
# Let's try a different approach to find test cases
# First, let's locate all test files in the repository
fd "test.*\.rs$"
# Then search for any test-related code in the src directory
rg "#\[test\]" -A 10 "src/"
# And look for any usage of to_fixed or to_significant in the entire codebase
rg "to_(fixed|significant)" --type rust
Length of output: 45746
|
@shuhuiluo everything is okay, but the default for rounding should be roundDown not roundHalfUp due to safety |
Revert your change. The default is ROUND_HALF_UP. |
it's been wrong since lmaoo |
You don't understand. |
Most people round half up for display, i.e. 0.465 to 0.47. It has nothing to do with safety. |
that's not how i think about it tbh, imagine something like 3.5 a round half will take it to 4 while a round down will take it to 3 |
So what? What's the purpose of |
|
to_fixed in my opinion is different tbh, but we should just do it the way uniswap did it |
This commit updates the rounding parameters in multiple modules to use
Option<Rounding>with a default value if none is specified. This change simplifies method calls and improves code flexibility by allowing optional rounding strategies.Summary by CodeRabbit
Release Notes
New Features
Nonefor rounding inCurrencyAmount,FractionBase,Percent, andPricestructs.Bug Fixes
Documentation