Skip to content

Conversation

@nabalone
Copy link
Contributor

@nabalone nabalone commented Nov 4, 2025

This change is Reviewable

@github-actions
Copy link

github-actions bot commented Nov 4, 2025

Palaso Tests

     4 files       4 suites   10m 11s ⏱️
 5 070 tests  4 837 ✅ 233 💤 0 ❌
16 525 runs  15 805 ✅ 720 💤 0 ❌

Results for commit bdd7fb5.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@imnasnainaec imnasnainaec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@imnasnainaec reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions


SIL.WritingSystems/IetfLanguageTag.cs line 1318 at r1 (raw file):

				if (uiLanguageCode == "en")
					return kSimplifiedChineseNameInEnglish;
				return kSimplifiedChineseAutonym;

Using kSimplifiedChineseAutonym (resp. kTraditionalChineseAutonym) for all uiLanguageCode != "en" is a decision that removes the possibility of other ui languages having their own translation. The changelog and/or these method summaries and/or the above comments should probably reflect that decision.


SIL.WritingSystems/IetfLanguageTag.cs line 1604 at r1 (raw file):

				case "中文(台湾)":
				case "中文(台湾)":
					return kTraditionalChineseAutonym;

There are two function which call on FixBotchedNativeName and this pr entirely handles the (generalCode == ChineseSimplifiedTag) and (generalCode == ChineseTraditionalTag) cases before that call. So it's reasonable to remove the various Chinese cases here. However, there are a few reasons to consider leaving them:

  1. They appear to cover known botches, so leaving them allows for a more robust function that can be used if the calling functions are later refactored.

  2. They help cover odd language code situations, such as zh-Hans-CN, which would give generalCode = zh (rather than zh-CN).

  3. For consistency with the درى case that's still here, even though (generalCode == "prs") is handled rather than calling FixBotchedNativeName.

@andrew-polk
Copy link
Contributor

SIL.WritingSystems/IetfLanguageTag.cs line 1604 at r1 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

There are two function which call on FixBotchedNativeName and this pr entirely handles the (generalCode == ChineseSimplifiedTag) and (generalCode == ChineseTraditionalTag) cases before that call. So it's reasonable to remove the various Chinese cases here. However, there are a few reasons to consider leaving them:

  1. They appear to cover known botches, so leaving them allows for a more robust function that can be used if the calling functions are later refactored.

  2. They help cover odd language code situations, such as zh-Hans-CN, which would give generalCode = zh (rather than zh-CN).

  3. For consistency with the درى case that's still here, even though (generalCode == "prs") is handled rather than calling FixBotchedNativeName.

This code is all such a mess...
I would be in favor of removing for two reasons:

  1. Anything we can do to simplify code in this area seems like a win.
  2. I'm pretty sure this was recently added as we were trying to work through these same issues but didn't quite get things right.

@nabalone nabalone force-pushed the BL-15436-anotherAttemptChineseAutonymErrors branch from 1711603 to 3aac426 Compare December 3, 2025 18:39
Copy link
Contributor Author

@nabalone nabalone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @andrew-polk and @imnasnainaec)


SIL.WritingSystems/IetfLanguageTag.cs line 1318 at r1 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

Using kSimplifiedChineseAutonym (resp. kTraditionalChineseAutonym) for all uiLanguageCode != "en" is a decision that removes the possibility of other ui languages having their own translation. The changelog and/or these method summaries and/or the above comments should probably reflect that decision.

Done. At least the behavior is consistent with the "prs" handling below. Though not sure that's worth much of anything in this woefully inconsistent langtag class anyways


SIL.WritingSystems/IetfLanguageTag.cs line 1604 at r1 (raw file):

Previously, andrew-polk wrote…

This code is all such a mess...
I would be in favor of removing for two reasons:

  1. Anything we can do to simplify code in this area seems like a win.
  2. I'm pretty sure this was recently added as we were trying to work through these same issues but didn't quite get things right.

Ah yes, I it added in https://github.com/sillsdev/libpalaso/pull/1471/files#diff-9fbf1831740234538b84fb58e65fa89be3a8e66268c987ba023e4289b869b284; and this PR is a better fix which we think should replace that fix

Copy link
Contributor

@andrew-polk andrew-polk left a 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 r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @nabalone)

@andrew-polk andrew-polk force-pushed the BL-15436-anotherAttemptChineseAutonymErrors branch from 3aac426 to bdd7fb5 Compare December 5, 2025 22:38
@andrew-polk
Copy link
Contributor

@imnasnainaec Did you want another look?

Copy link
Contributor

@imnasnainaec imnasnainaec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@imnasnainaec reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nabalone)


SIL.WritingSystems/IetfLanguageTag.cs line 1298 at r3 (raw file):
The summary says

return the autonym (if the ui language matches) or the English name

but isn't the new behavior to return autonym or (if the UI language is English) the English name?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants