Skip to content

Conversation

bobzhang
Copy link
Contributor

No description provided.

Copy link

Potential zero-padding issue in hexadecimal output

Category
Correctness
Code Snippet
sb.write_string(b.to_int().to_string(radix=16))
Recommendation
Use zero-padding to ensure consistent 2-digit hex format: b.to_int().to_string(radix=16).pad_start(2, '0')
Reasoning
The original code manually handled each hex digit to ensure 2-character output (e.g., '0a' instead of 'a'). The new implementation may produce single-digit hex values for bytes < 16, which could break parsing or display expectations.

Inconsistent hex formatting between Unicode and byte escapes

Category
Correctness
Code Snippet
logger..write_string("\u{")..write_string(code.to_string(radix=16))..write_char('}')
Recommendation
Consider padding Unicode hex values: code.to_string(radix=16).pad_start(expected_width, '0')
Reasoning
Unicode escape sequences typically expect consistent formatting. While not necessarily incorrect, the original manual digit approach ensured predictable output length.

Good refactoring that eliminates code duplication

Category
Maintainability
Code Snippet
Removal of to_hex_digit function and its usage across multiple files
Recommendation
Consider adding unit tests to verify hex formatting behavior remains consistent
Reasoning
The refactoring successfully removes a custom utility function in favor of standard library functionality, improving maintainability. However, the behavioral change in padding should be tested to ensure compatibility.

builtin/json.mbt Outdated
sb.write_string("\\x")
sb.write_char(to_hex_digit(b.to_int() / 16))
sb.write_char(to_hex_digit(b.to_int() % 16))
sb.write_string(b.to_int().to_string(radix=16))
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing zero padding here.

@bobzhang bobzhang force-pushed the hongbo/bytesview_to_json branch from d2f6487 to a02edf5 Compare September 23, 2025 05:59
cursor[bot]

This comment was marked as outdated.

@bobzhang bobzhang force-pushed the hongbo/bytesview_to_json branch from a02edf5 to a7c3338 Compare September 23, 2025 06:12
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 1296

Details

  • 11 of 11 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.005%) to 89.464%

Totals Coverage Status
Change from base Build 1293: 0.005%
Covered Lines: 9289
Relevant Lines: 10383

💛 - Coveralls

@bobzhang bobzhang merged commit 0927339 into main Sep 23, 2025
15 checks passed
@bobzhang bobzhang deleted the hongbo/bytesview_to_json branch September 23, 2025 06:26
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.

3 participants