-
-
Notifications
You must be signed in to change notification settings - Fork 6
fix: BL-15209 autonyms should be in the default script #114
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
StephenMcConnel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment that may not apply to this specific change.
It may be better for at least one other person to review this who is more familiar with this code.
@StephenMcConnel reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nabalone)
components/language-chooser/common/find-language/scripts/langtagProcessingHelpers.ts line 57 at r1 (raw file):
// These are not in the langtags.json file but may be added in the processing isRepresentativeForMacrolanguage: boolean; indivIsoCode: string;
would a comment on how this differs from iso639_3 be helpful?
andrew-polk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrew-polk reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nabalone)
components/language-chooser/common/find-language/scripts/langtagProcessing.ts line 175 at r1 (raw file):
alternativeTags: [...langData.alternativeTags], parentMacrolanguage: langData.parentMacrolanguage, isMacrolanguage: false, // we add macrolanguages separately below. See macrolanguageNotes.md
Is this fixing an unrelated bug?
(Same question for removing it above)
032afd1 to
15e6092
Compare
andrew-polk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrew-polk reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nabalone)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an issue where autonyms (native language names) should be displayed using the default script of a language rather than from other script variants. The change ensures that when multiple script entries exist for a language, the system prioritizes the autonym from the default script entry (the one without script modifiers in the tag).
- Added logic to track and prefer default script autonyms during language data processing
- Updated data structures to include a
defaultScriptAutonymfield for proper autonym selection - Added comprehensive test coverage for the new default script autonym behavior
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| langtagProcessingHelpers.ts | Added interface fields and removed debug logging |
| langtagProcessing.ts | Implemented core logic for tracking and using default script autonyms |
| languageSearch.spec.ts | Added tests to verify default script autonym selection behavior |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
15e6092 to
a1ddb9b
Compare
nabalone
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @andrew-polk and @StephenMcConnel)
components/language-chooser/common/find-language/scripts/langtagProcessing.ts line 175 at r1 (raw file):
Previously, andrew-polk wrote…
Is this fixing an unrelated bug?
(Same question for removing it above)
Kindof, more of a refactor. The old code produced basically the same result but in unnecessarily roundabout way - by replacing the iso639-3 codes with individual language codes and then checking again whether these replaced codes were macrolanguage codes, which will always come out false (except for possibly the few pathological cases which will need fixing up regardless.)
components/language-chooser/common/find-language/scripts/langtagProcessingHelpers.ts line 57 at r1 (raw file):
Previously, StephenMcConnel (Steve McConnel) wrote…
would a comment on how this differs from iso639_3 be helpful?
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
andrew-polk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrew-polk reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @StephenMcConnel)
andrew-polk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrew-polk dismissed @StephenMcConnel from a discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @nabalone)
This change is