Conversation
`left-pad`, `right-pad`, and `pad` measured length with `String#length` (UTF-16 code units), so a surrogate pair or other multi-code-unit grapheme counted as more than one character. In `pad`, the truncation step went further: it sliced with `String#substring` at a code-unit offset that could fall inside a surrogate pair, producing an unpaired surrogate in the output. Switch all three to measure and slice by grapheme cluster (using the `num-grapheme-clusters` and `base/slice-grapheme-clusters` primitives already in this package), so length always means user-perceived characters and truncation never splits one apart. Fixes stdlib-js#298
If `pad` is a string whose grapheme cluster count doesn't grow when repeated -- a bare Unicode combining mark, since per UAX stdlib-js#29 GB9 a run of Extend characters merges into one cluster no matter how many times it repeats -- left-pad/right-pad can return a string shorter than the requested length. Pin the current behavior with a test and note it in both READMEs rather than leave it undocumented.
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #298.
Description
This pull request:
left-pad,right-pad(both the public packages and theirbase/implementations), andpadmeasure and slice by grapheme cluster (user-perceived character) instead ofString#length(UTF-16 code unit), using thenum-grapheme-clustersandbase/slice-grapheme-clustersprimitives that already exist in this package.pad's truncation step usedString#substringat a code-unit offset, which could land inside a surrogate pair and emit an unpaired surrogate (invalid UTF-16) in the output — e.g.pad('a', 2, {rpad: '😀'})returned'a\ud83d'. Truncating by grapheme cluster instead means this can no longer happen.padwith astral characters to confirm no lone surrogate is produced.padis a bare Unicode combining mark, repeating it does not grow the grapheme cluster count — per UAX #29 GB9, a run ofExtendcharacters with no preceding base merges into a single cluster regardless of length — so the output can fall short of the requested length. Credit to @Shubham-Padkonde, who found this exact edge case in this comment while investigating the same issue; I've verified it reproduces (lpad('a', 5, '̈')currently returns a 2-cluster result,rpad('a', 5, '̈')a 1-cluster result) and left it as a documented limitation rather than folding a fix into this PR — see the Questions section below.Related Issues
This pull request has the following related issues:
Questions
@Shubham-Padkonde raised two things worth a maintainer call before merge:
Bug, notRFC, and code-unit-based length was never a documented design choice) and kept the diff to swapping the length metric, not new options or a separate API. But it does change output for any existing caller padding non-ASCII text, so if the maintainers see it as a breaking change underCONTRIBUTING.md's RFC process, happy to split this into an RFC first.RangeErrorfor apadstring that can't make progress, or is documenting the limitation (as this PR does) enough?Other
No.
Checklist
AI Assistance
If you answered "yes" above, how did you use AI assistance?
Disclosure
This PR was written primarily by Claude Code.
@stdlib-js/reviewers