Panic occurs when formatting with separator and some format specifier #6213

yt2b · 2025-10-24T01:44:33Z

Summary

#6212
I've fixed get_separator_interval.
And I've added extra tests.

>>>>> f"{123456:,g}"
'123,456'
>>>>> f"{123456:,e}"
'1.234560e+05'
>>>>> f"{123456:,%}"
'12,345,600.000000%'

Summary by CodeRabbit

New Features
- Extended thousands-separator support to decimal, fixed-point, general/scientific, and percentage numeric formats.
Tests
- Added tests verifying thousands-separator formatting across floating-point and integer specifiers, scientific and percentage formats, and signed/uppercase variations.

coderabbitai · 2025-10-24T01:44:47Z

Walkthrough

Extended magnitude-separator logic to return a separator interval of 3 for additional numeric format types (Decimal, FixedPoint, GeneralFormat, Exponent, Percentage) and added tests exercising thousands-separator format specifiers for floats and integers.

Changes

Cohort / File(s)	Summary
Format separator logic `common/src/format.rs`	`get_separator_interval` now returns `3` for `Decimal`, `FixedPoint`, `GeneralFormat`, `Exponent`, and `Percentage` format kinds in addition to existing Binary/Octal/Hex handling. Non-number formats still panic; `None` remains `3`.
Format specification tests `extra_tests/snippets/builtin_format.py`	Added tests for thousands-separator aliases (`,g`, `,G`, `,e`, `,E`, `,%`) covering floating and integer formatting, signed and case variants, and scientific/percentage outputs.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Caller as Formatting code
    participant GetSep as get_separator_interval
    note over GetSep #D3E4CD: Updated matching logic
    Caller->>GetSep: request separator interval for FormatKind
    alt FormatKind in {Binary, Octal, Hex}
        GetSep-->>Caller: returns 3
    else FormatKind in {Decimal, FixedPoint, GeneralFormat, Exponent, Percentage}
        GetSep-->>Caller: returns 3
    else FormatKind == None
        GetSep-->>Caller: returns 3
    else
        GetSep-->>Caller: panic (non-number format)
    end

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

youknowone

Poem

🐰 I hop through digits, dots, and signs,
Placing commas in tidy lines.
Exponents, percent — I tidy the yield,
Thousands united across the field.

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 pull request title "Panic occurs when formatting with separator and some format specifier" directly describes the issue that the PR addresses. The changeset extends the magnitude-separator logic to support additional numeric format types (Decimal, FixedPoint, GeneralFormat, Exponent, and Percentage) that previously would have caused a panic when used with separators. The title clearly identifies the main problem being fixed and is specific enough that a reviewer would understand the intent, making it a valid problem-statement style title for a bug fix.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e27ccc5 and 8c86bc7.

📒 Files selected for processing (1)

common/src/format.rs (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

common/src/format.rs

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.}

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9825d8a and e27ccc5.

📒 Files selected for processing (2)

common/src/format.rs (1 hunks)
extra_tests/snippets/builtin_format.py (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

extra_tests/**/*.py

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

extra_tests/**/*.py: Follow PEP 8 style for custom Python code
Use ruff to lint custom Python code

Files:

extra_tests/snippets/builtin_format.py

**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Format Rust code with the default rustfmt style (run cargo fmt)
Run clippy and fix any warnings or lints introduced by your changes
Follow Rust best practices for error handling and memory management

Files:

common/src/format.rs

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)

GitHub Check: Check the WASM package and demo
GitHub Check: Run snippets and cpython tests on wasm-wasi
GitHub Check: Run snippets and cpython tests (macos-latest)
GitHub Check: Run tests under miri
GitHub Check: Run snippets and cpython tests (windows-latest)
GitHub Check: Run snippets and cpython tests (ubuntu-latest)
GitHub Check: Run rust tests (windows-latest)
GitHub Check: Run rust tests (ubuntu-latest)
GitHub Check: Run rust tests (macos-latest)
GitHub Check: Check Rust code with rustfmt and clippy
GitHub Check: Ensure compilation on various targets

🔇 Additional comments (1)

extra_tests/snippets/builtin_format.py (1)

191-195: LGTM! Comprehensive test coverage for new separator support.

The new test cases properly validate separator functionality with GeneralFormat (g/G), Exponent (e/E), and Percentage (%) format types. The expected outputs are correct:

For general format with 123456, the separator applies to the integer representation

For exponent format, no separator appears because the mantissa "1" is too short

For percentage, the separator correctly applies to the scaled value (12,345,600)

Good coverage of both uppercase and lowercase format specifiers.

common/src/format.rs

ShaharNaveh · 2025-10-24T10:11:30Z

@yt2b this looks very good, I think that it's worth adding those tests upstream (to cpython). I find it odd that they don't have a test for it

youknowone

👍 Thank you!

yt2b added 2 commits October 24, 2025 10:35

Fix get_separator_interval

33c41bb

Add extra tests

e27ccc5

coderabbitai bot reviewed Oct 24, 2025

View reviewed changes

common/src/format.rs Show resolved Hide resolved

Remove FormatType::Number from match arm

8c86bc7

youknowone requested a review from ShaharNaveh October 24, 2025 04:14

ShaharNaveh approved these changes Oct 24, 2025

View reviewed changes

youknowone approved these changes Oct 24, 2025

View reviewed changes

youknowone merged commit 1d23f6e into RustPython:main Oct 24, 2025
12 checks passed

yt2b deleted the fix_formatting branch October 24, 2025 12:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Panic occurs when formatting with separator and some format specifier #6213

Panic occurs when formatting with separator and some format specifier #6213

Uh oh!

yt2b commented Oct 24, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Oct 24, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

ShaharNaveh commented Oct 24, 2025

Uh oh!

youknowone left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Panic occurs when formatting with separator and some format specifier #6213

Panic occurs when formatting with separator and some format specifier #6213

Uh oh!

Conversation

yt2b commented Oct 24, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ShaharNaveh commented Oct 24, 2025

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yt2b commented Oct 24, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Oct 24, 2025 •

edited

Loading