Throw if the library is used before being initialized (BL-13266)

[andrew-polk]

+semver: major

---

This change is 

[github-actions]

Test Results

0 tests  ±0   0 ✔️ ±0   0s ⏱️ ±0s
0 suites ±0   0 💤 ±0 
0 files   ±0   0 ❌ ±0 

Results for commit 6880426. ± Comparison against base commit 5e2b24b.

♻️ This comment has been updated with latest results.

[StephenMcConnel]

Looks good with a couple of questions that don't necessarily require any code changes. Possibly a comment in the test code, but maybe not even that.

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @andrew-polk)

---

src/L10NSharpTests/LocalizationManagerTests_NoManagersLoaded.cs line 33 at r1 (raw file):

				LocalizationManager.UILanguageId = "en";
				Assert.That(
					LocalizationManager.GetDynamicString("Glom", "prefix.data", "data"),

So I assume that GetDynamicString (and GetDynamicStringOrEnglish) do not use the code paths that trap and report the error? Is that because they both provide the name/id of the specific LocalizationManager needed, and presumably loaded since the programmer knows about it?

[andrew-polk]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @StephenMcConnel)

---

src/L10NSharpTests/LocalizationManagerTests_NoManagersLoaded.cs line 33 at r1 (raw file):

Previously, StephenMcConnel (Steve McConnel) wrote…

So I assume that GetDynamicString (and GetDynamicStringOrEnglish) do not use the code paths that trap and report the error? Is that because they both provide the name/id of the specific LocalizationManager needed, and presumably loaded since the programmer knows about it?

Actually, it's because that code path has the following:

			//this happens in unit test environments or apps that
			//have imported a library that is L10N'ized, but the app
			//itself isn't initializing L10N yet.
			if (LoadedManagers.Count == 0)
			{
				if (PreviouslyLoadedManagers.Contains(appId))
				{
					if (LocalizationManager.ThrowIfManagerDisposed)
					{
						throw new ObjectDisposedException(
							$"The application id '{appId}' refers to a LocalizationManagerInternal that has been disposed");
					}
					return string.IsNullOrEmpty(englishText) ? id : englishText;
				}

				if (!string.IsNullOrEmpty(englishText) && langId == LocalizationManager.kDefaultLang)
					return englishText;
				return id;
			}

which I didn't want to attempt to mess with.

The good thing is that code path doesn't set the mapping, so it doesn't actually break anything.

I'm definitely open to suggestions on how to improve commenting around this.

[jasonleenaylor]

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @StephenMcConnel)

---

src/L10NSharpTests/LocalizationManagerTests_NoManagersLoaded.cs line 94 at r1 (raw file):

				Thread.CurrentThread.CurrentUICulture = new System.Globalization.CultureInfo(cultureName);
				//LocalizationManager.UILanguageId = cultureName;

Was this commented line left in for a reason?

[andrew-polk]

src/L10NSharpTests/LocalizationManagerTests_NoManagersLoaded.cs line 94 at r1 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…

Was this commented line left in for a reason?

Nope! Fixed.

[jasonleenaylor]

Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @StephenMcConnel)

[tombogle]

src/L10NSharpTests/LocalizationManagerTests_NoManagersLoaded.cs line 33 at r1 (raw file):

Previously, andrew-polk wrote…

Actually, it's because that code path has the following:

			//this happens in unit test environments or apps that
			//have imported a library that is L10N'ized, but the app
			//itself isn't initializing L10N yet.
			if (LoadedManagers.Count == 0)
			{
				if (PreviouslyLoadedManagers.Contains(appId))
				{
					if (LocalizationManager.ThrowIfManagerDisposed)
					{
						throw new ObjectDisposedException(
							$"The application id '{appId}' refers to a LocalizationManagerInternal that has been disposed");
					}
					return string.IsNullOrEmpty(englishText) ? id : englishText;
				}

				if (!string.IsNullOrEmpty(englishText) && langId == LocalizationManager.kDefaultLang)
					return englishText;
				return id;
			}

which I didn't want to attempt to mess with.

The good thing is that code path doesn't set the mapping, so it doesn't actually break anything.

I'm definitely open to suggestions on how to improve commenting around this.

It would probably be safe to check that flagg and throw that same InvalidOperationException if there are 0 loaded managers and 0 previously loaded managers. But it's probably okay to just leave it as is.