fix(format): isolate special grouping rule to sign-aware zero-padding #5924
Conversation
WalkthroughThe update refines the logic for inserting grouping separators and zero-padding in integer formatting, ensuring correct behavior when width, zero-padding, and alignment are combined. The changes specifically adjust the calculation of displayed digit count based on alignment and padding, and introduce new tests to verify compliance with CPython's formatting rules. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant FormatSpec
participant add_magnitude_separators
Caller->>FormatSpec: format integer with width, grouping, zero-padding, alignment
FormatSpec->>add_magnitude_separators: call with magnitude, width, fill, align
add_magnitude_separators->>add_magnitude_separators: calculate disp_digit_cnt (consider fill and align)
add_magnitude_separators-->>FormatSpec: return formatted string with correct padding/grouping
FormatSpec-->>Caller: return formatted integer string
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ever0de, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request addresses a long-standing bug (issue #5922) in number formatting within the format.rs module. The core problem was an incorrect interaction between width specifiers and grouping separators, particularly when zero-padding was not explicitly requested. My changes refactor the padding logic to ensure that width-based padding for non-zero-padded numbers correctly applies as external space padding, rather than influencing the internal digit count for grouping. This brings the formatting behavior in line with CPython's specification, improving the accuracy of f-string formatting for integers.
Highlights
- Formatting Logic Refinement: I've modified the
add_magnitude_separatorsfunction incommon/src/format.rsto correctly handle width-based padding for numbers with grouping. Thedisp_digit_cntcalculation now differentiates between scenarios where zero-padding is explicitly requested (using the '0' flag) and when it is not. This ensures that width specifiers for non-zero-padded numbers apply as external space padding, rather than affecting the internal digit count for grouping. - Test Coverage: I've added a new Rust test case,
test_format_int_width_and_grouping, incommon/src/format.rsto specifically validate the fix for issue #5922. Additionally, a corresponding Python snippet test has been included inextra_tests/snippets/builtin_format.pyto confirm thatf"{1234:10,}"produces the expected space-padded output, aligning with CPython's behavior.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request addresses issue #5922 by ensuring correct padding for numbers with width and grouping in the formatting logic. It introduces a check for zero-padding to determine how width is calculated when grouping separators are present. Additionally, it includes a new test case to validate the fix and an assertion in a python test file.
common/src/format.rs
Outdated
| let magnitude_len = magnitude_str.len(); | ||
| let width = self.width.unwrap_or(magnitude_len) as i32 - prefix.len() as i32; | ||
| let disp_digit_cnt = cmp::max(width, magnitude_len as i32); | ||
|
|
||
| let disp_digit_cnt = if zero_padding { | ||
| let width = self.width.unwrap_or(magnitude_len) as i32 - prefix.len() as i32; | ||
| cmp::max(width, magnitude_len as i32) | ||
| } else { | ||
| magnitude_len as i32 | ||
| }; |
There was a problem hiding this comment.
The magnitude_len variable is declared on line 439 but not used when zero_padding is false, consider moving the declaration inside the if zero_padding block to avoid confusion and improve readability.
let disp_digit_cnt = if zero_padding {
let width = self.width.unwrap_or(magnitude_len) as i32 - prefix.len() as i32;
cmp::max(width, magnitude_len as i32)
} else {
let magnitude_len = magnitude_str.len();
magnitude_len as i32
};There was a problem hiding this comment.
I'll keep it as is, since magnitude_len is used in both branches.
|
Oh, I am so sorry. I realize now that I misunderstood and mistook you for a maintainer. |
This comment was marked as resolved.
This comment was marked as resolved.
|
I can update the code to use this condition: let disp_digit_cnt = if self.fill == Some('0'.into())
&& self.align == Some(FormatAlign::AfterSign)
{
// ...EDIT) // CPython behavior: f'{1234:0>10,}' results in "000001,234"
let spec = FormatSpec::parse("0>10,").unwrap();
let result = spec.format_int(&BigInt::from(1234)).unwrap();
assert_eq!(result, "000001,234");I will now patch the code to check for both conditions (AfterSign && '0') to handle this correctly. |
ever0de
force-pushed
the
fix/format-separator-padding
branch
from
ddb39b8 to
9a2d099
Compare
ever0de
changed the title
ever0de
changed the title
Summary by CodeRabbit
Bug Fixes
Tests
Summary by CodeRabbit
Bug Fixes
Tests