Match locale names longest-first so a name that is a prefix of another parses - #130
Open
spokodev wants to merge 2 commits into
Open
Match locale names longest-first so a name that is a prefix of another parses#130spokodev wants to merge 2 commits into
spokodev wants to merge 2 commits into
Conversation
…r parses
formatRe joined a field's names into an ordered regex alternation
(?:a|b|…). JavaScript alternation returns the first alternative that
matches at the position, not the longest. When an earlier name is a
prefix of a later name in the same field, the parser matched the short
name first and the leftover characters then broke the parse.
This corrupts d3's own formatter output in shipped locales: Czech
"červen" (June) is a prefix of "červenec" (July), so
timeParse("%d %B %Y") of "15 červenec 2020" returned null; Turkish
"Pazar" (Sun) is a prefix of "Pazartesi" (Mon), so every Monday/Saturday
failed to parse.
Sort the names by descending length before building the alternation, so
the longest matching name wins. formatLookup is keyed by the matched
text, so the reordering does not affect index resolution.
Fil
reviewed
Jul 24, 2026
Fil
left a comment
Member
There was a problem hiding this comment.
lgtm, but can you please tighten the comments? They contain redundant explanations.
For example I would strip "; ordered alternation is first-match, not longest-match. formatLookup is keyed by text, so order is irrelevant to it." and " Ordered alternation matched the shorter name first, so these formatted dates failed to round-trip."
Author
|
Trimmed both — kept the sort rationale and the example names, dropped the rest. |
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
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.
Problem
formatRebuilds a parser for a locale field (months, weekdays, periods) byjoining the names into an ordered regex alternation:
JavaScript alternation is first-match, not longest-match:
(?:a|b|…)returns the first alternative that matches at the position. When an earlier
name is a prefix of a later name in the same field, the parser matches the
short name first, and the leftover characters then break the parse.
This silently corrupts d3's own formatter output in shipped locales:
"červen"(June) is a prefix of"červenec"(July). Same failure for Turkish"Pazar"(Sun) vs"Pazartesi"(Mon), and"Cuma"(Fri) vs"Cumartesi"(Sat): a
utcParse("%Y %U %A")round-trip over 2020 intr-TRfails on all104 Mondays and Saturdays.
vi-VNmonths ("Tháng 1"vs"Tháng 10/11/12")are affected too.
Fix
Sort the names by descending length before building the alternation, so the
longest matching name wins.
formatLookupis keyed by the matched text, so thereordering does not affect index resolution:
.slice()keeps the caller's array (shared withformatLookup) unmutated.Test
Added a
test/locale-test.jscase that formats and re-parses a date in the twoshipped locales with the prefix collisions (Czech July, Turkish Monday). Fails
on
main, passes with the fix. Verified no regression: en-US full/short monthand weekday round-trips over all months/days still pass, and
eslintis clean.