Add string utility functions - #33
Conversation
SummaryThis PR adds a new What reviewers should knowFunctions added:
Things to check:
|
![sonar-review-dev18[bot]](https://avatars.githubusercontent.com/in/3054993?s=60&v=4){kind=small}
| def to_snake_case(text): | ||
| result = [] | ||
| for i, char in enumerate(text): | ||
| if char.isupper() and i > 0: | ||
| result.append('_') | ||
| result.append(char.lower()) | ||
| return ''.join(result) |
There was a problem hiding this comment.
Bug: to_snake_case does not handle consecutive uppercase letters (acronyms). Each uppercase character unconditionally gets its own underscore prefix, so "XMLParser" → "x_m_l_parser" instead of "xml_parser".
If the function needs to handle real-world identifiers, consecutive uppercase runs need to be treated as a single word. A common approach: only insert _ when transitioning from a lowercase character to an uppercase one, or when a run of uppercase letters is followed by a lowercase letter.
- Mark as noise
|
|
||
| def pad_left(text, width, char=' '): | ||
| if len(text) >= width: | ||
| return text |
There was a problem hiding this comment.
Bug: char is not validated to be a single character. If the caller passes a multi-character string (e.g. pad_left("x", 5, "ab")), char * (width - len(text)) produces a string of the wrong length and the result won't respect width. Raise ValueError if len(char) != 1.
| return text | |
| if len(char) != 1: | |
| raise ValueError("char must be a single character") | |
| if len(text) >= width: | |
| return text |
- Mark as noise
| def pad_left(text, width, char=' '): | ||
| if len(text) >= width: |
There was a problem hiding this comment.
Missing test coverage: No test covers the guard clause (len(text) >= width) or the multi-char char edge case. Unit tests with len(text) == width, len(text) > width, and char of length > 1 would catch the regression above and confirm the guard returns the original string unchanged.
- Mark as noise
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
SonarQube reviewer guide
|
There was a problem hiding this comment.
One bug in truncate needs fixing before merge (see inline). The to_snake_case consecutive-uppercase issue from the earlier review round is also still open.
SonarQube Cloud status: Coverage on new code is 0% against an 80% threshold — see the SonarQube reviewer guide above for details.
| return ' '.join(text.split()[::-1]) | ||
|
|
||
|
|
||
| def truncate(text, max_length, suffix='...'): | ||
| if len(text) <= max_length: | ||
| return text | ||
| return text[:max_length - len(suffix)] + suffix |
There was a problem hiding this comment.
Bug: When max_length is smaller than len(suffix) (e.g. truncate("hello", 2)), max_length - len(suffix) is negative. text[:-1] slices from the end rather than the start, so the result is longer than max_length and contains wrong content.
Guard against this:
- If
max_length <= len(suffix), either raiseValueErroror returnsuffix[:max_length]— depending on the intended contract.
| return ' '.join(text.split()[::-1]) | |
| def truncate(text, max_length, suffix='...'): | |
| if len(text) <= max_length: | |
| return text | |
| return text[:max_length - len(suffix)] + suffix | |
| def truncate(text, max_length, suffix='...'): | |
| if max_length < len(suffix): | |
| raise ValueError("max_length must be >= len(suffix)") | |
| if len(text) <= max_length: | |
| return text | |
| return text[:max_length - len(suffix)] + suffix |
- Mark as noise
1 participant
No description provided.