Skip to content

Conversation

@ericyangpan
Copy link

Add CommaNumberFormatter to format numbers with thousand separators. Update Progress.zig to use commaNumber formatter for byte counts, replacing {Bi:.2} format with comma-separated numbers for better readability (e.g., 1,234,567 instead of 1.23M).

What does this PR do?

This PR adds a new CommaNumberFormatter to src/fmt.zig that formats numbers with thousand separators (commas) for improved readability. The formatter is then integrated into src/Progress.zig to display byte counts in a more human-readable format.

Changes:

  • Added CommaNumberFormatter struct and commaNumber() function in src/fmt.zig
  • Updated Progress.zig to use commaNumber formatter for byte count displays instead of the {Bi:.2} format
  • Numbers are now displayed with comma separators (e.g., 1,234,567 instead of 1.23M)

This change improves the readability of large numbers in progress output, making it easier to understand the exact byte counts at a glance.

How did you verify your code works?

  1. Created standalone test file: Created test_comma_number.zig with comprehensive test cases covering:

    • Edge cases (0, single digits, numbers < 1000)
    • Numbers requiring commas (1000, 1234, 12345, etc.)
    • Large numbers (1,234,567,890)
    • All tests pass with zig test test_comma_number.zig
  2. Test cases verified:

    • 0"0"
    • 123"123" (no comma needed)
    • 1234"1,234"
    • 1234567"1,234,567"
    • 1234567890"1,234,567,890"
  3. Integration testing: The formatter is used in Progress.zig for displaying byte counts in progress indicators, replacing the previous {Bi:.2} format with comma-separated numbers.

Add CommaNumberFormatter to format numbers with thousand separators.
Update Progress.zig to use commaNumber formatter for byte counts,
replacing {Bi:.2} format with comma-separated numbers for better
readability (e.g., 1,234,567 instead of 1.23M).
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 5, 2025

Walkthrough

Adds a comma-separated integer formatter and updates the bytes progress display to use that formatter, changing byte-count rendering while leaving control flow and non-bytes branches unchanged. (≤50 words)

Changes

Cohort / File(s) Summary
Number formatter
src/fmt.zig
Added CommaNumberFormatter (pub const) and commaNumber(value: usize) helper; implements format to write a usize with thousand separators.
Progress bytes rendering
src/Progress.zig
Replaced previous compact bytes formatting with output that uses bun.fmt.commaNumber for current and total byte counts in progress display; control flow unchanged.

Possibly related PRs

Suggested reviewers

  • pfgithub
  • cirospaciari
  • taylordotfish

Pre-merge checks

✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding a comma number formatter and integrating it into the progress display.
Description check ✅ Passed The description follows the template with both required sections completed. 'What does this PR do?' provides clear details about the changes, and 'How did you verify your code works?' documents comprehensive test cases and verification methods.

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between dd84117 and 6312b76.

📒 Files selected for processing (1)
  • src/Progress.zig (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,zig}

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

src/**/*.{cpp,zig}: Use bun bd or bun run build:debug to build debug versions for C++ and Zig source files; creates debug build at ./build/debug/bun-debug
Run tests using bun bd test <test-file> with the debug build; never use bun test directly as it will not include your changes
Execute files using bun bd <file> <...args>; never use bun <file> directly as it will not include your changes
Enable debug logs for specific scopes using BUN_DEBUG_$(SCOPE)=1 environment variable
Code generation happens automatically as part of the build process; no manual code generation commands are required

Files:

  • src/Progress.zig
src/**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

Use bun.Output.scoped(.${SCOPE}, .hidden) for creating debug logs in Zig code

Implement core functionality in Zig, typically in its own directory in src/

src/**/*.zig: Private fields in Zig are fully supported using the # prefix: struct { #foo: u32 };
Use decl literals in Zig for declaration initialization: const decl: Decl = .{ .binding = 0, .value = 0 };
Prefer @import at the bottom of the file (auto formatter will move them automatically)

Be careful with memory management in Zig code - use defer for cleanup with allocators

Files:

  • src/Progress.zig
**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)

**/*.zig: Expose generated bindings in Zig structs using pub const js = JSC.Codegen.JS<ClassName> with trait conversion methods: toJS, fromJS, and fromJSDirect
Use consistent parameter name globalObject instead of ctx in Zig constructor and method implementations
Use bun.JSError!JSValue return type for Zig methods and constructors to enable proper error handling and exception propagation
Implement resource cleanup using deinit() method that releases resources, followed by finalize() called by the GC that invokes deinit() and frees the pointer
Use JSC.markBinding(@src()) in finalize methods for debugging purposes before calling deinit()
For methods returning cached properties in Zig, declare external C++ functions using extern fn and callconv(JSC.conv) calling convention
Implement getter functions with naming pattern get<PropertyName> in Zig that accept this and globalObject parameters and return JSC.JSValue
Access JavaScript CallFrame arguments using callFrame.argument(i), check argument count with callFrame.argumentCount(), and get this with callFrame.thisValue()
For reference-counted objects, use .deref() in finalize instead of destroy() to release references to other JS objects

Files:

  • src/Progress.zig
🧠 Learnings (2)
📚 Learning: 2025-10-24T10:43:09.398Z
Learnt from: fmguerreiro
Repo: oven-sh/bun PR: 23774
File: src/install/PackageManager/updatePackageJSONAndInstall.zig:548-548
Timestamp: 2025-10-24T10:43:09.398Z
Learning: In Bun's Zig codebase, the `as(usize, intCast(...))` cast pattern triggers a Zig compiler bug that causes compilation to hang indefinitely when used in complex control flow contexts (loops + short-circuit operators + optional unwrapping). Avoid this pattern and use simpler alternatives like just `intCast(...)` if type casting is necessary.

Applied to files:

  • src/Progress.zig
📚 Learning: 2025-09-02T19:17:26.376Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 0
File: :0-0
Timestamp: 2025-09-02T19:17:26.376Z
Learning: In Bun's Zig codebase, when handling error unions where the same cleanup operation (like `rawFree`) needs to be performed regardless of success or failure, prefer using boolean folding with `else |err| switch (err)` over duplicating the cleanup call in multiple switch branches. This approach avoids code duplication while maintaining compile-time error checking.

Applied to files:

  • src/Progress.zig
🔇 Additional comments (1)
src/Progress.zig (1)

332-337: The review comment is based on code changes that do not exist in the current repository. The actual code at lines 330-350 in src/Progress.zig uses Zig's built-in binary format specifier {Bi:.2} (e.g., [{Bi:.2}/{Bi:.2}]), not bun.fmt.commaNumber(). Additionally, searching the entire repository yields no results for commaNumber or CommaNumberFormatter, indicating these functions do not exist in the codebase. The review appears to address proposed changes that were not implemented or merged.

Likely an incorrect or invalid review 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
Contributor

@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: 2

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 4d60b6f and dd84117.

📒 Files selected for processing (2)
  • src/Progress.zig (1 hunks)
  • src/fmt.zig (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,zig}

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

src/**/*.{cpp,zig}: Use bun bd or bun run build:debug to build debug versions for C++ and Zig source files; creates debug build at ./build/debug/bun-debug
Run tests using bun bd test <test-file> with the debug build; never use bun test directly as it will not include your changes
Execute files using bun bd <file> <...args>; never use bun <file> directly as it will not include your changes
Enable debug logs for specific scopes using BUN_DEBUG_$(SCOPE)=1 environment variable
Code generation happens automatically as part of the build process; no manual code generation commands are required

Files:

  • src/Progress.zig
  • src/fmt.zig
src/**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

Use bun.Output.scoped(.${SCOPE}, .hidden) for creating debug logs in Zig code

Implement core functionality in Zig, typically in its own directory in src/

src/**/*.zig: Private fields in Zig are fully supported using the # prefix: struct { #foo: u32 };
Use decl literals in Zig for declaration initialization: const decl: Decl = .{ .binding = 0, .value = 0 };
Prefer @import at the bottom of the file (auto formatter will move them automatically)

Be careful with memory management in Zig code - use defer for cleanup with allocators

Files:

  • src/Progress.zig
  • src/fmt.zig
**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)

**/*.zig: Expose generated bindings in Zig structs using pub const js = JSC.Codegen.JS<ClassName> with trait conversion methods: toJS, fromJS, and fromJSDirect
Use consistent parameter name globalObject instead of ctx in Zig constructor and method implementations
Use bun.JSError!JSValue return type for Zig methods and constructors to enable proper error handling and exception propagation
Implement resource cleanup using deinit() method that releases resources, followed by finalize() called by the GC that invokes deinit() and frees the pointer
Use JSC.markBinding(@src()) in finalize methods for debugging purposes before calling deinit()
For methods returning cached properties in Zig, declare external C++ functions using extern fn and callconv(JSC.conv) calling convention
Implement getter functions with naming pattern get<PropertyName> in Zig that accept this and globalObject parameters and return JSC.JSValue
Access JavaScript CallFrame arguments using callFrame.argument(i), check argument count with callFrame.argumentCount(), and get this with callFrame.thisValue()
For reference-counted objects, use .deref() in finalize instead of destroy() to release references to other JS objects

Files:

  • src/Progress.zig
  • src/fmt.zig
🔇 Additional comments (1)
src/fmt.zig (1)

1484-1518: LGTM! The comma number formatter implementation is correct and efficient.

The logic correctly handles:

  • Zero case with early return
  • Buffer sizing (32 bytes is sufficient for max usize with commas)
  • First group calculation ensuring 1-3 leading digits
  • Loop that safely processes remaining digits in groups of 3

The mathematical guarantee that (len - remaining) is always divisible by 3 ensures the loop never overruns the buffer.

Consolidate multiple bufWrite calls into single calls for byte unit
progress display. This makes the code more concise and easier to read
without changing functionality.
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