Skip to content

fix: align long ToMetric decimals with int behavior (#1699)#1795

Closed
wyf027 wants to merge 2 commits into
Humanizr:mainfrom
wyf027:fix/long-tometric-decimals-1699-dev
Closed

fix: align long ToMetric decimals with int behavior (#1699)#1795
wyf027 wants to merge 2 commits into
Humanizr:mainfrom
wyf027:fix/long-tometric-decimals-1699-dev

Conversation

@wyf027

@wyf027 wyf027 commented May 25, 2026

Copy link
Copy Markdown

Summary

  • For long values below the first metric prefix, use the double ToMetric path when decimals is specified.
  • Fixes ((long)1).ToMetric(decimals: 5) returning "1.00000" instead of "1" like int.

Fixes #1699

Test plan

  • Added LongToMetricDecimalsMatchIntBehavior for values 1, 42, 999
  • Existing TestAllSymbolsAsLong cases unchanged for prefixed values
  • CI green on main

For values below the first metric prefix, delegate to the double path
when decimals are specified instead of padding fixed decimal places.

Co-authored-by: Cursor <cursoragent@cursor.com>
@coderabbitai

coderabbitai Bot commented May 25, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

BuildRepresentation(long, ...) now delegates to the double ToMetric path when decimals > 0; tests were updated and new tests added to assert parity with int behavior and to adjust expected formatting outputs.

Changes

Align long.ToMetric decimals handling with int behavior

Layer / File(s) Summary
Delegate long decimals to double ToMetric
src/Humanizer/MetricNumeralExtensions.cs
BuildRepresentation(long, ...) delegates to ((double)input).ToMetric(formats, decimals) when decimals > 0 instead of producing a fixed trailing-zero decimal string.
Update and add tests for decimals parity
tests/Humanizer.Tests/MetricNumeralTests.cs, tests/Humanizer.Tests/CoverageGapTests.cs
Adjusted ToMetricUsesDefaultIntFormatting expectations (no trailing zeros for whole numbers), added LongToMetricDecimalsMatchIntBehavior theory and an out-of-range Fact for rounding limits, and updated one coverage assertion expecting integer-form output.

🎯 2 (Simple) | ⏱️ ~8 minutes

🐰 A tiny hop to fix the score,
Long and int now match once more,
Decimals heed precision's call,
No stray zeros trailing small,
Harmony in metrics restored.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: aligning long.ToMetric decimals behavior with int behavior and fixing the specific inconsistency mentioned in the issue.
Linked Issues check ✅ Passed The pull request fully addresses issue #1699 by modifying long.ToMetric to delegate to the double path when decimals is specified, eliminating fixed zero-padded decimals and restoring parity with int.ToMetric behavior.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing the long.ToMetric decimals issue: implementation changes in MetricNumeralExtensions.cs, test updates validating the fix, and test assertion updates reflecting the corrected behavior.
Description check ✅ Passed The pull request description is clearly related to the changeset and provides specific context about the fix for aligning long ToMetric decimals behavior with int behavior.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b53386267e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +335 to +338
if (decimals > 0)
{
return ((double)input).ToMetric(formats, decimals);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Preserve decimal padding for non-prefixed long metrics

Routing long values with decimals > 0 through the double overload drops trailing zeroes for values below 1000, because the double path rounds and then formats with default ToString semantics. That changes outputs like 123L.ToMetric(decimals: 1) from "123.0" to "123" (same for 0), which regresses existing long-format expectations and will break consumers that rely on fixed decimal precision in this range.

Useful? React with 👍 / 👎.

Update sub-prefix long cases and coverage assertions to match the
double rounding path used by int.ToMetric, including decimals > 15.

Co-authored-by: Cursor <cursoragent@cursor.com>
@wyf027

wyf027 commented May 26, 2026

Copy link
Copy Markdown
Author

Pushed test fixes for Azure CI (2bc1c636):

  • Updated TestAllSymbolsAsLong sub-prefix expectations to match int.ToMetric rounding (no zero-padding for values like 123 / 0).
  • Added LongToMetricDecimalsMatchIntBehaviorWhenRoundingLimitExceeded for decimals: 20.
  • Updated CoverageGapTests assertion for 123L.ToMetric(WithSpace, 1)"123 ".

These align with issue #1699 semantics. Re-running CI.

@wyf027

wyf027 commented May 26, 2026

Copy link
Copy Markdown
Author

Azure CI is green after test expectation updates (2bc1c636). Ready for maintainer review.

@wyf027

wyf027 commented May 27, 2026

Copy link
Copy Markdown
Author

Closing in favor of #1700 per @logiclrd's review on #1699.

#1700 covers the full long.ToMetric decimal semantics (including scaled values like 123456L.ToMetric(decimals: 20)123.456k), not just the unscaled [-999, 999] path. #1700 is rebased, CI green, and CodeRabbit approved.

Sorry for the duplicate — should have coordinated with the existing PR.

@wyf027

wyf027 commented May 27, 2026

Copy link
Copy Markdown
Author

Superseded by #1700.

@wyf027 wyf027 closed this May 27, 2026
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.

long.ToMetric interprets decimals as a required number of fixed decimals instead of desired precision

1 participant