add BCP-47 validation cases ported from libpalaso - #156
Conversation
Broaden isValidBcp47Tag coverage with well-formedness cases drawn from libpalaso's IetfLanguageTag tests: full subtag-position tags, extlang limits, Unicode/transform extensions, private-use length limits, and empty-subtag/illegal-character rejections. Also add a doc comment clarifying that isValidBcp47Tag checks syntactic validity only, not registry membership (unlike libpalaso's IsValid). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Devin says components/language-chooser/common/find-language/languageTagUtils.spec.ts:R822 Extlang-limit test does not test the extlang limit
|
andrew-polk
left a comment
There was a problem hiding this comment.
@andrew-polk reviewed 2 files and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on hahn-kev).
Drop the abcdefgh-abc-def-ghi-jkl case: an 8-letter primary language subtag cannot carry any extlang in our grammar, so it was rejected for that reason rather than for exceeding three extlangs and tested nothing. en-abc-def-ghi-jkl (2-letter primary + four extlangs) exercises the limit directly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
andrew-polk
left a comment
There was a problem hiding this comment.
@andrew-polk reviewed 1 file and all commit messages, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on hahn-kev).
I was checking to see if using ICU4X Locale could improve our isValid check (it didn't), but as part of that I imported the BCP-47 validation tests from libpalaso, so I figured I'd open a PR to contribute those tests. Feel free to dump it if it doesn't seem useful.
AI summary
Broadens test coverage for
isValidBcp47Tagin@ethnolib/find-languagewith well-formedness cases ported from libpalaso'sIetfLanguageTagtests (SIL.WritingSystems.Tests/IetfLanguageTagTests.cs). No behavior change to the validator itself — the implementation still uses the existing BCP-47 grammar regex.Why: We evaluated swapping
isValidBcp47Tagto an ICU4X-backed parser to see if it was more complete than the regex. It wasn't: ICU4X follows UTS-35, a strict subset of BCP-47, so it rejects well-formed tags the library itself emits (extlang tags likesgn-ads-GH, grandfathered tags likei-ami, 4–8-char primary languages, and language-lessx-tags). Wrapping it with a regex fallback made it behaviorally identical to the plain regex — pure cost, no gain — so the ICU swap was dropped. The libpalaso test corpus surfaced during that comparison was worth keeping.Changes:
languageTagUtils.spec.ts: +6 test cases coveringtpi-AR,tpi-Lepc-BR-fonipa-x-blah,qed-Lepc-x-rubbish,qed)en-US-u-ca-gregory,en-t-jp,de-DE-u-co-phonebk)-,--,en--US,qed-?~, trailingx-)languageTagUtils.ts: doc comment clarifying thatisValidBcp47Tagchecks syntactic validity only, not registry membership — unlike libpalaso's registry-backedIetfLanguageTag.IsValid.Note on scope: libpalaso's
IsValidadditionally validates subtags against the IANA/ISO registries (so e.g.zzzis invalid there). That semantic level is deliberately out of scope here; these tests only assert well-formedness, matching the existingisValidBcp47Tagcontract.Test plan
npm run testonceincomponents/language-chooser/common/find-language— 88 tests pass (was 82).npx tsc --noEmit— clean.This change is