Bump L10NSharp to -beta0004; Target net8

[imnasnainaec]

This change is 

[tombogle]

Presumably, if all goes well, we'll want to release the new version of L10nSharp and upgrade to that before releasing a new version of libpalaso.

@tombogle reviewed 6 files and all commit messages, and made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on imnasnainaec).

[github-actions]

Palaso Tests

     4 files  ±    0       4 suites  ±0   8m 22s ⏱️ - 1m 47s
 5 101 tests +    6   4 867 ✅ +    6  234 💤 ± 0  0 ❌ ±0 
15 030 runs   - 1 567  14 367 ✅  - 1 509  663 💤  - 58  0 ❌ ±0 

Results for commit 37d9b71. ± Comparison against base commit 7677eed.

♻️ This comment has been updated with latest results.

[tombogle]

@tombogle reviewed 11 files and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on imnasnainaec).

[imnasnainaec]

@tombogle Is this a concern?

TestApps/SIL.Windows.Forms.TestApp/SIL.Windows.Forms.TestApp.csproj:R5

SIL.Windows.Forms.TestApp may have Geckofx compatibility issues with new net8.0-windows target

Adding <TargetFrameworks>$(TargetFrameworks);net8.0-windows</TargetFrameworks> at TestApps/SIL.Windows.Forms.TestApp/SIL.Windows.Forms.TestApp.csproj:5 means this app now also builds for net8.0-windows. However, it has direct PackageReferences to Geckofx45 (lines 19-22) which are old .NET Framework packages. The conditions on those references only check OS and architecture, not target framework. This may cause NuGet restore warnings or build issues when targeting net8.0-windows, since Geckofx45 likely has no compatible assets for that TFM. It probably falls back gracefully, but it's worth verifying the build succeeds cleanly.

[tombogle]

@tombogle Is this a concern?

TestApps/SIL.Windows.Forms.TestApp/SIL.Windows.Forms.TestApp.csproj:R5
SIL.Windows.Forms.TestApp may have Geckofx compatibility issues with new net8.0-windows target
Adding <TargetFrameworks>$(TargetFrameworks);net8.0-windows</TargetFrameworks> at TestApps/SIL.Windows.Forms.TestApp/SIL.Windows.Forms.TestApp.csproj:5 means this app now also builds for net8.0-windows. However, it has direct PackageReferences to Geckofx45 (lines 19-22) which are old .NET Framework packages. The conditions on those references only check OS and architecture, not target framework. This may cause NuGet restore warnings or build issues when targeting net8.0-windows, since Geckofx45 likely has no compatible assets for that TFM. It probably falls back gracefully, but it's worth verifying the build succeeds cleanly.

We obviously don't deploy the test app, so If it builds cleanly both locally and on CI, and you can run it (locally) with no obvious issues, I'd say we're good. It is likely that nothing in the test app exercises the control(s) that depend on Geckofx, but I'm not sure while controls those are. Even if it does, it still might work.

[tombogle]

Looks like there are some problems, so I'll hold off on my re-review.

[imnasnainaec]

Looks like there are some problems, so I'll hold off on my re-review.

@tombogle Unrelated error. Reran the failed test suite and it passed.

[tombogle]

@tombogle reviewed 5 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on imnasnainaec).

---

SIL.Core.Desktop.Tests/i18n/L10NSharpLocalizerTests.cs line 12 at r4 (raw file):

{
	[TestFixture]
	public class L10NSharpLocalizerTests

I'm a little unclear as to what these tests are really testing. Based on the commit comment and the removal of the test app, I perceive that ostensibly this is just a check that says that we've correctly referenced an instance of L10nSharp that is compatible with the test fixture's runtime. Assuming that is really what we're testing, we should state that. If so, is there any actual point in testing so many methods? All of them are for "unknown" keys, but I don't know if this is actually working in a way that accurately simulates real life for a normal app. In a normal app, creating the LocalizationManager causes it to scan the codebase, at which point all of the keys (except the dynamic one) become "known". I'm guessing this is probably also happening for this test. But without stepping through the debugger to see, we'd never know the difference in these tests since we're always requesting the English string, which s always just the value passed in. The only case for which the key definitely remains unknown is the one for GetIsStringAvailableForLangId, since the code scanning logic is not programmed to pick it up.

[imnasnainaec]

@imnasnainaec reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: 12 of 13 files reviewed, 1 unresolved discussion (waiting on tombogle).

---

SIL.Core.Desktop.Tests/i18n/L10NSharpLocalizerTests.cs line 12 at r4 (raw file):

Previously, tombogle (Tom Bogle) wrote…

I'm a little unclear as to what these tests are really testing. Based on the commit comment and the removal of the test app, I perceive that ostensibly this is just a check that says that we've correctly referenced an instance of L10nSharp that is compatible with the test fixture's runtime. Assuming that is really what we're testing, we should state that. If so, is there any actual point in testing so many methods? All of them are for "unknown" keys, but I don't know if this is actually working in a way that accurately simulates real life for a normal app. In a normal app, creating the LocalizationManager causes it to scan the codebase, at which point all of the keys (except the dynamic one) become "known". I'm guessing this is probably also happening for this test. But without stepping through the debugger to see, we'd never know the difference in these tests since we're always requesting the English string, which s always just the value passed in. The only case for which the key definitely remains unknown is the one for GetIsStringAvailableForLangId, since the code scanning logic is not programmed to pick it up.

Good point. I was trying to just add simple tests to ensure the Core.Desktop wrapper of L10NSharp works, but that ended up being a bunch of silly fallback tests. I've removed two of the more redundant ones and added a translation test. Though I'm still not familiar enough with what's going on to know if it's satisfactorily simulating a real life situation.

[tombogle]

SIL.Core.Desktop.Tests/i18n/L10NSharpLocalizerTests.cs line 78 at r5 (raw file):

			// Write the English XLIFF for L10NSharp to use as a baseline. Without it, the
			// trans-unit id is absent on a clean machine, so L10NSharp sees it as an orphan and
			// silently drops it from the French XLIFF.

This is only true because you're failing to tag the string in the French XLIFF file as dynamic. For localized strings obtained using a call to one of the GetDynamicString... methods, the XLIFF entry should be something like this:

Code snippet:

      <trans-unit id=""{kKey}"" sil:dynamic="true">
        <source xml:lang=""en"">{kEnglish}</source>
		<target xml:lang=""fr"" state=""final"">{kFrench}</target>

[tombogle]

@tombogle reviewed 1 file and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on imnasnainaec).

---

SIL.Core.Desktop.Tests/i18n/L10NSharpLocalizerTests.cs line 12 at r4 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

Good point. I was trying to just add simple tests to ensure the Core.Desktop wrapper of L10NSharp works, but that ended up being a bunch of silly fallback tests. I've removed two of the more redundant ones and added a translation test. Though I'm still not familiar enough with what's going on to know if it's satisfactorily simulating a real life situation.

Add a

XML comment for the test fixture class that explains the purpose.

---

SIL.Core.Desktop.Tests/i18n/L10NSharpLocalizerTests.cs line 14 at r5 (raw file):

	public class L10NSharpLocalizerTests
	{
		private string _tempDir;

You probably want to use SIL.TestUtilities.TemporaryFolder. It already handles cleaning up the folder when disposed.

---

SIL.Core.Desktop.Tests/i18n/L10NSharpLocalizerTests.cs line 57 at r5 (raw file):

		{
			var result = _localizer.GetIsStringAvailableForLangId("NoSuch.Key", kLang);
			Assert.That(result, Is.False);

To make this more meaningful, add a test case showing that _localizer.GetIsStringAvailableForLangId("NoSuch.Key.Static", kLang) returns true. If that works, you'll have succeeded in demonstrating that the code scanning logic works for the test, because it will have picked up the key you use in GetString_UnknownKey_ReturnsFallbackEnglish. (Of course, then you might want to change the key in both places to not say "NoSuch".)

[imnasnainaec]

@imnasnainaec made 2 comments and resolved 2 discussions.
Reviewable status: 12 of 15 files reviewed, 2 unresolved discussions (waiting on tombogle).

---

SIL.Core.Desktop.Tests/i18n/L10NSharpLocalizerTests.cs line 12 at r4 (raw file):

Previously, tombogle (Tom Bogle) wrote…

Add a

XML comment for the test fixture class that explains the purpose.

Done.

---

SIL.Core.Desktop.Tests/i18n/L10NSharpLocalizerTests.cs line 78 at r5 (raw file):

Previously, tombogle (Tom Bogle) wrote…

This is only true because you're failing to tag the string in the French XLIFF file as dynamic. For localized strings obtained using a call to one of the GetDynamicString... methods, the XLIFF entry should be something like this:

Done.

[tombogle]

@tombogle reviewed 4 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on imnasnainaec).

---

SIL.Core.Desktop.Tests/i18n/L10NSharpLocalizerTests.cs line 108 at r7 (raw file):

			using var frManager = LocalizationManager.Create("fr", kFrAppId, kFrAppName, "1.0",
				_tempFolder.Path, null, new[] { "SIL." }, null);

This is fine now, as is. I think you could still potentially simplify it somewhat by just using the existing LM. Probably for it to actually pick up the French xlf file, it would have to be created during setup (before creating the LM). In some ways, perhaps it is best to have this test completely self-contained and isolated like this. The only think I sort of don't like is that if someone were to look at this text code to see an example of how the localization manager was intended to be used, they could get the mistaken impression that it was necessary or advisable to create a distinct one for each target language, and that really isn't the case.

[tombogle]

@tombogle resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on imnasnainaec).

[imnasnainaec]

@imnasnainaec made 1 comment and resolved 1 discussion.
Reviewable status: 14 of 15 files reviewed, all discussions resolved (waiting on tombogle).

---

SIL.Core.Desktop.Tests/i18n/L10NSharpLocalizerTests.cs line 108 at r7 (raw file):

Previously, tombogle (Tom Bogle) wrote…

This is fine now, as is. I think you could still potentially simplify it somewhat by just using the existing LM. Probably for it to actually pick up the French xlf file, it would have to be created during setup (before creating the LM). In some ways, perhaps it is best to have this test completely self-contained and isolated like this. The only think I sort of don't like is that if someone were to look at this text code to see an example of how the localization manager was intended to be used, they could get the mistaken impression that it was necessary or advisable to create a distinct one for each target language, and that really isn't the case.

Legit concern. I added a comment.

[tombogle]

@tombogle reviewed 1 file and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on imnasnainaec).