Skip to content

Conversation

Yu-zh
Copy link
Collaborator

@Yu-zh Yu-zh commented Apr 27, 2025

No description provided.

@Yu-zh Yu-zh closed this Apr 27, 2025
Copy link

Prefix handling logic change may affect backward compatibility

Category
Correctness
Code Snippet
['0', 'x' | 'X', .. rest] if base == 16 => (16, rest, true)
Recommendation
Consider adding a comment explaining the behavior change where '0b01' with base=16 is now valid and parsed as 2817
Reasoning
The new behavior where prefix is treated as part of the number when base doesn't match might be surprising. This should be clearly documented as it appears to be an intentional change based on the new test cases.

Test cases could be better organized

Category
Maintainability
Code Snippet
test "more edge cases" { ... }
Recommendation
Split the test cases into logical groups with descriptive names like 'test_prefix_with_matching_base' and 'test_prefix_with_non_matching_base'
Reasoning
The current test organization makes it harder to understand what specific behaviors are being tested. Grouping related test cases would improve readability and maintenance.

Base range check should be moved earlier

Category
Correctness
Code Snippet
_ => if base is (1..=36) { (base, view, false) } else { base_err!() }
Recommendation
Move the base range check to the start of the function to fail fast:

fn check_and_consume_base(...) {
  if base != 0 && !(base is (1..=36)) { base_err!() }
  // rest of the function
}
**Reasoning**
Validating the base parameter early makes the function more robust and easier to reason about. Currently, the base range check only happens in one code path.

</details>

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 6461

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.01%) to 92.682%

Totals Coverage Status
Change from base Build 6455: 0.01%
Covered Lines: 6117
Relevant Lines: 6600

💛 - Coveralls

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