Skip to content

Conversation

@Eh2406
Copy link

@Eh2406 Eh2406 commented Dec 24, 2025

A small PR, to learn the process and etiquette with this project.

print_fixedp did not take full advantage of the formatting options int write! dsl. By inlining it into the one place it was called, a bunch of code could be made simpler.

Summary by CodeRabbit

  • Refactor
    • Simplified Decimal display formatting logic by streamlining the internal implementation and removing redundant code structures.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 24, 2025

📝 Walkthrough

Walkthrough

Display formatting for the Decimal type in pallas-math was refactored. The fmt method now inlines logic to compute integer and fractional parts using div_rem, handle sign explicitly, and normalize negative values. The separate print_fixedp helper function was removed.

Changes

Cohort / File(s) Summary
Decimal Display Refactoring
pallas-math/src/math_dashu.rs
Reworked fmt method for Display trait: replaced print_fixedp helper call with inline logic using div_rem to split integer and fractional parts, explicit sign handling via conditional minus write, normalization of negative quotients/remainders to absolute values, and formatted output with precision-based width. Removed now-unused print_fixedp free function.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 Whiskers twitch with joy—no more helpers to call,
The fmt method now handles it all!
Div and rem dance, signs align just right,
Decimal formats shine, crisp and bright!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'refactor(math): clean up fmt' accurately describes the main change: refactoring the Display formatting code for Decimal by inlining the print_fixedp helper and simplifying the fmt method.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
pallas-math/src/math_dashu.rs (1)

43-55: Consider simplifying sign handling with abs().

The logic is correct but could be more idiomatic. Instead of negating temp_q and temp_r conditionally, you could use abs():

🔎 Proposed simplification using abs()
 let is_negative = self.data < ZERO.value;
-let (mut temp_q, mut temp_r) = (&self.data).div_rem(&self.precision_multiplier);
+let (temp_q, temp_r) = (&self.data).abs().div_rem(&self.precision_multiplier);

 if is_negative {
     f.write_str("-")?;
 }

-if temp_q < ZERO.value {
-    temp_q = temp_q.neg();
-}
-if temp_r < ZERO.value {
-    temp_r = temp_r.neg();
-}
 write!(
     f,

This computes the absolute value once before div_rem, eliminating the need to check and negate both quotient and remainder separately.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2aba7ff and fe3a596.

📒 Files selected for processing (1)
  • pallas-math/src/math_dashu.rs
🧰 Additional context used
🧬 Code graph analysis (1)
pallas-math/src/math_dashu.rs (1)
pallas-math/src/math.rs (1)
  • precision (37-37)
🔇 Additional comments (1)
pallas-math/src/math_dashu.rs (1)

41-62: LGTM! Clean refactoring that inlines the formatting logic.

The refactoring successfully inlines the print_fixedp helper, making fuller use of the write! formatting DSL. The logic correctly handles sign, computes integer and fractional parts, and formats with proper zero-padding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant