Add flag to return only approved translations (BL-5472)

[StephenMcConnel]

This change is 

[andrew-polk]

Reviewed 3 of 3 files at r1. Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

src/L10NSharpTests/XLiffUtilsTests.cs, line 106 at r1 (raw file):

          Assert.AreEqual("Ocultar el botón que abre la configuración.", source);
          Assert.IsTrue(esdoc.File.Body.TranslationsById.TryGetValue("SettingsProtection.PasswordDialog.FactoryPassword", out source));
          Assert.AreEqual("Factory Password", source);    // note it's stored and returned even though marked as needing translation

Is the desirable or de facto behavior? I find it strange that unapproved strings (with flag set) return false but untranslated strings return true with English. I would expect the same behavior in both cases.

---

Comments from Reviewable

[StephenMcConnel]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

src/L10NSharpTests/XLiffUtilsTests.cs, line 106 at r1 (raw file):

Previously, andrew-polk wrote…

Is the desirable or de facto behavior? I find it strange that unapproved strings (with flag set) return false but untranslated strings return true with English. I would expect the same behavior in both cases.

If you look at the actual file, the English string exists as the Spanish "translation" with an attribute indicating that it needs translation. This is how Crowdin behaves, so I copied it in the test data. In general, getting English back when no translation has been done is the de facto way that we've been using L10NSharp.

---

Comments from Reviewable

[andrew-polk]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

src/L10NSharpTests/XLiffUtilsTests.cs, line 106 at r1 (raw file):

Previously, StephenMcConnel (Steve McConnel) wrote…

If you look at the actual file, the English string exists as the Spanish "translation" with an attribute indicating that it needs translation. This is how Crowdin behaves, so I copied it in the test data. In general, getting English back when no translation has been done is the de facto way that we've been using L10NSharp.

Right. I was trying to figure out why we want this new behavior to be different or if we do.

---

Comments from Reviewable

[StephenMcConnel]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

src/L10NSharpTests/XLiffUtilsTests.cs, line 106 at r1 (raw file):

Previously, andrew-polk wrote…

Right. I was trying to figure out why we want this new behavior to be different or if we do.

Strings in English as such come in from source instead of target, and are always implicitly both "translated" and "approved", even though explicitly neither flag would be set.

---

Comments from Reviewable

[andrew-polk]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

src/L10NSharpTests/XLiffUtilsTests.cs, line 106 at r1 (raw file):

Previously, StephenMcConnel (Steve McConnel) wrote…

Strings in English as such come in from source instead of target, and are always implicitly both "translated" and "approved", even though explicitly neither flag would be set.

Since the end result is always going to be the same (displaying English), I am not going to keep harping on this.

I was just pointing out that from a consistency perspective, a caller to TryGetValue would expect to get the same result regardless of the reason for which a translation is unavailable.

The other reason this isn't worth belaboring is that the "right" thing to do is probably to change the original implementation such that TryGetValue returns false if it would return English regardless of the reason. But that is beyond the scope of this change.

---

Comments from Reviewable