[API] #109: find specific languages when generics are specified

[papeh]

If a client tries to initialize a LocalizationManager with only
a language code, and we have a language available with that code
plus extra information, use that language without prompting the user.
Previous behavior: prompted the user with a dialog in this case.
Unchanged behavior: initializing with extra information will fall back to the bare language code.

This addresses #109
This addresses https://jira.sil.org/browse/LT-21232

---

This change is 

[github-actions]

Test Results

151 tests  +3   146 ✔️ +3   9s ⏱️ -2s
  24 suites +1       5 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit 297ffc9. ± Comparison against base commit 23ff95c.

This pull request removes 14 and adds 17 tests. Note that renamed tests count towards both.

GetString_OverloadThatTakesListOfLanguages_WorksWithFolders_1
GetString_OverloadThatTakesListOfLanguages_WorksWithFolders_2
GetString_OverloadThatTakesListOfLanguages_WorksWithFolders_3
GetString_OverloadThatTakesListOfLanguages_WorksWithFolders_4
GetString_OverloadThatTakesListOfLanguages_WorksWithFolders_5
GetString_OverloadThatTakesListOfLanguages_WorksWithFolders_6
GetString_OverloadThatTakesListOfLanguages_WorksWithFolders_7
GetString_OverloadThatTakesListOfLanguages_Works_1
GetString_OverloadThatTakesListOfLanguages_Works_2
GetString_OverloadThatTakesListOfLanguages_Works_3
…

TestMappingLanguageCodesToAvailable_AmbiguousOptions_PromptsUser("zh-CN")
TestMappingLanguageCodesToAvailable_AmbiguousOptions_PromptsUser("zh-TW")
TestMappingLanguageCodesToAvailable_FindsSpecificGivenGeneric
ar,en; finds en
ar,en; finds en in folders
ar,fr,en; finds fr
ar,fr,en; finds fr in folders
en,fr; finds en
en,fr; finds en in folders
en; finds en
…

♻️ This comment has been updated with latest results.

[tombogle]

I think you'll want an entry in CHANGELOG.md for this somewhat subtle change.

[tombogle]

src/L10NSharp/XLiffUtils/XLiffLocalizedStringCache.cs line 179 at r1 (raw file):

				// Another possibility is that the lang folder is "es-ES" but the client is requesting only "es".
				// TODO: actual implementation here:
				if (langId == "zz") return;

Although there is one exception, it seems our normal standard is to put the return on a line by itself. IMHO, that makes for more readable code. It's easy to miss the return at the end of the line when scanning through the code quickly.

[tombogle]

src/L10NSharp/XLiffUtils/XLiffLocalizedStringCache.cs line 178 at r1 (raw file):

					return;
				// Another possibility is that the lang folder is "es-ES" but the client is requesting only "es".
				// TODO: actual implementation here:

Can you expand on what this TODO is about? I'm guessing the idea is that someday we might know of specific language tag for which we can do something better than just give up. Is that the idea?

[tombogle]

src/L10NSharpTests/LocalizationManagerTestsBase.cs line 438 at r1 (raw file):

		[TestCase(new[] { "en" }, "blahInEnglishCode", "en", TestName = "en; finds en")]
		[TestCase(new[] { "en", "fr" }, "blahInEnglishCode", "en", TestName = "en,fr; finds en")]
		[TestCase(new[] { "ar", "en" }, "blahInEnglishCode", "en", TestName = "ar,en; finds en")] // our arabic doesn't have a translation of 'blah', so fall to the code's English

Not your fault, but this should say "fall back"

[tombogle]

src/L10NSharpTests/LocalizationManagerTestsBase.cs line 581 at r1 (raw file):

		}

		private void AddChineseChineseTranslation(string folderPath)

Not sure I understand the purpose of having Chinese twicetwice in the method name. zh-CN is Simplified Chinese as used in China. Perhaps the name should be AddSimplifiedChineseTranslation OR AddSimplifiedChineseOfChinaTranslation OR AddChineseOfChinaTranslation OR AddZhCnChineseTranslation OR AddChineseZhCnTranslation OR AddChineseTranslation.

[tombogle]

src/L10NSharpTests/LocalizationManagerTestsBase.cs line 987 at r1 (raw file):

				// Check asking for a specific form of the language when we have only a different specific form.
				var str = LocalizationManager.GetString("theId", ".", "", new []{ "zh" }, out var languageIdUsed);

I know you said you intended to leaved "undefined" the behavior when "zn" is requested, but multiple zn- variants are available. Specifically in the case of "zn", I'm not sure this is a good idea. I don't know when/where we expect an application to request "zn", but I think it will be very common that if there is an available "zn-" language, there will be at least two. If it is truly impossible to decide upon a defined order of precedence, then I can't think of a benefit of implementing this behavior. If a caller can't know what to expect, why would they call the method?

[tombogle]

src/L10NSharpTests/XLiffLocalizedStringCacheTests.cs line 86 at r1 (raw file):

		public void TryGetDocument()
		{
			//var sut = new XLiffLocalizedStringCache();

Is this unfinished work, or something that needs to be reverted?

[tombogle]

Reviewed 3 of 4 files at r1, all commit messages.
Reviewable status: 3 of 4 files reviewed, 6 unresolved discussions (waiting on @papeh)

[papeh]

Dismissed @tombogle from a discussion.
Reviewable status: 1 of 4 files reviewed, 5 unresolved discussions (waiting on @papeh and @tombogle)

---

src/L10NSharp/XLiffUtils/XLiffLocalizedStringCache.cs line 178 at r1 (raw file):

Previously, tombogle (Tom Bogle) wrote…

Can you expand on what this TODO is about? I'm guessing the idea is that someday we might know of specific language tag for which we can do something better than just give up. Is that the idea?

I plan to implement the behaviour for the remaining cases before removing this PR's draft status.

---

src/L10NSharp/XLiffUtils/XLiffLocalizedStringCache.cs line 179 at r1 (raw file):

Previously, tombogle (Tom Bogle) wrote…

Although there is one exception, it seems our normal standard is to put the return on a line by itself. IMHO, that makes for more readable code. It's easy to miss the return at the end of the line when scanning through the code quickly.

That line was temporary. It has been removed.

---

src/L10NSharpTests/LocalizationManagerTestsBase.cs line 438 at r1 (raw file):

Previously, tombogle (Tom Bogle) wrote…

Not your fault, but this should say "fall back"

will do

---

src/L10NSharpTests/LocalizationManagerTestsBase.cs line 581 at r1 (raw file):

Previously, tombogle (Tom Bogle) wrote…

Not sure I understand the purpose of having Chinese twicetwice in the method name. zh-CN is Simplified Chinese as used in China. Perhaps the name should be AddSimplifiedChineseTranslation OR AddSimplifiedChineseOfChinaTranslation OR AddChineseOfChinaTranslation OR AddZhCnChineseTranslation OR AddChineseZhCnTranslation OR AddChineseTranslation.

will do.

---

src/L10NSharpTests/LocalizationManagerTestsBase.cs line 987 at r1 (raw file):

Previously, tombogle (Tom Bogle) wrote…

I know you said you intended to leaved "undefined" the behavior when "zn" is requested, but multiple zn- variants are available. Specifically in the case of "zn", I'm not sure this is a good idea. I don't know when/where we expect an application to request "zn", but I think it will be very common that if there is an available "zn-" language, there will be at least two. If it is truly impossible to decide upon a defined order of precedence, then I can't think of a benefit of implementing this behavior. If a caller can't know what to expect, why would they call the method?

How about I add a test that it returns a match only if there is exactly one?

---

src/L10NSharpTests/XLiffLocalizedStringCacheTests.cs line 86 at r1 (raw file):

Previously, tombogle (Tom Bogle) wrote…

Is this unfinished work, or something that needs to be reverted?

unfinished work.

[papeh]

Reviewable status: 1 of 4 files reviewed, 5 unresolved discussions (waiting on @tombogle)

---

src/L10NSharpTests/LocalizationManagerTestsBase.cs line 987 at r1 (raw file):

Previously, papeh wrote…

How about I add a test that it returns a match only if there is exactly one?

(FieldWorks ships only zh-CN, because some systems (Mono and historically Windows) do not recognize zh on its own.

[tombogle]

src/L10NSharpTests/LocalizationManagerTestsBase.cs line 987 at r1 (raw file):

Previously, papeh wrote…

(FieldWorks ships only zh-CN, because some systems (Mono and historically Windows) do not recognize zh on its own.

I think that seems like a reasonable implementation (at least until we can find something more reasonable to do in some case).

[papeh]

Reviewable status: 1 of 5 files reviewed, 5 unresolved discussions (waiting on @tombogle)

---

src/L10NSharpTests/LocalizationManagerTestsBase.cs line 438 at r1 (raw file):

Previously, papeh wrote…

will do

Done.

---

src/L10NSharpTests/LocalizationManagerTestsBase.cs line 581 at r1 (raw file):

Previously, papeh wrote…

will do.

Done.

[papeh]

Reviewable status: 0 of 14 files reviewed, 5 unresolved discussions (waiting on @tombogle)

---

src/L10NSharpTests/XLiffLocalizedStringCacheTests.cs line 86 at r1 (raw file):

Previously, papeh wrote…

unfinished work.

this part of XLiffLocalizedStringCache is easier to test in LocalizationManagerTestsBase

[tombogle]

CHANGELOG.md line 23 at r6 (raw file):

- `LocalizationManager.Create` methods without `TranslationMemory kind` parameter

### Fixed

Even if no public members were changed, I wonder if we want this to be regarded as a "fix" or a "change"? Not 100% sure that these changes wouldn't be in some sense "breaking", given the new behavior. Hopefully not, but it's hard to be sure.

[tombogle]

L10NSharp.sln.DotSettings line 7 at r6 (raw file):

	<s:Boolean x:Key="/Default/UserDictionary/Words/=Filenames/@EntryIndexedValue">True</s:Boolean>
	<s:Boolean x:Key="/Default/UserDictionary/Words/=langs/@EntryIndexedValue">True</s:Boolean>
	<s:Boolean x:Key="/Default/UserDictionary/Words/=Liff/@EntryIndexedValue">True</s:Boolean>

I had considered doing this at one point, but since Liff isn't really a word and the miscapitalization is something that probably "should" be fixed if it weren't so entrenched, I kind of wondered if it was maybe better to just let it keep complaining.

[tombogle]

Just a few outstanding questions to ponder. Otherwise, LGTM. Since a lot of projects depend on this, might not be all bad to get another set of eyes to review it, maybe someone from the Bloom team.

Reviewed 2 of 4 files at r4, 3 of 12 files at r5, 11 of 11 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @papeh)

[papeh]

Reviewable status: 5 of 14 files reviewed, 2 unresolved discussions (waiting on @tombogle)

---

CHANGELOG.md line 23 at r6 (raw file):

Previously, tombogle (Tom Bogle) wrote…

Even if no public members were changed, I wonder if we want this to be regarded as a "fix" or a "change"? Not 100% sure that these changes wouldn't be in some sense "breaking", given the new behavior. Hopefully not, but it's hard to be sure.

Yes, it breaks the behaviour where creating a manager for "es" when only "es-ES" is available results in a user prompt. The only documentation I found (comment on LocalizationManagerInternal.MapToExistingLanguageIfPossible) says that the post-PR behaviour is correct (at least after es-ES has been loaded).

---

L10NSharp.sln.DotSettings line 7 at r6 (raw file):

Previously, tombogle (Tom Bogle) wrote…

I had considered doing this at one point, but since Liff isn't really a word and the miscapitalization is something that probably "should" be fixed if it weren't so entrenched, I kind of wondered if it was maybe better to just let it keep complaining.

I was about to fix them, but so many are public, and I couldn't be bothered to deprecate them and set up aliases.