Add IgnoreExistingEnglishXliffFiles global flag

[StephenMcConnel]

Also move XLiffDocument merging code to LocalizationManager. The new code helps in harvesting strings from running a program to verify which strings in existing xliff files may be outdated and which strings need to be added or updated.

---

This change is 

[JohnThomson]

I gather most of this is just moving code to a new location. Is there anything in particular that changed that I should look at? I don't think it's necessary to re-review code that just moved, so I'm happy for you just to merge unless you're doubtful about anything.

---

Review status: 0 of 6 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[StephenMcConnel]

The new code is mostly that in LocalizationManager.cs which revolves around IgnoreExistingEnglishXliffFiles.

---

Review status: 0 of 6 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[JohnThomson]

OK, I think I've had a reasonable look at the new stuff. A few minor and optional suggestions...up to you whether to act on any of them or just merge.

---

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

---

src/L10NSharp/LocalizationManager.cs, line 285 at r1 (raw file):

          if (!DefaultStringFileExistsAndHasContents() && File.Exists(defaultStringFileInstalledPath))
          {
              if (UILanguageId != "en" || !IgnoreExistingEnglishXliffFiles)

This expression or its negation occurs a few times and might be worth capturing in a method, especially since I think it has a quite specific purpose...something like "ScanningForCurrentStrings", right? That is, if the language is English and we're ignoring EnglishXliffFiles, we're using chunks of this code in a special way, not to provide localization of strings but to find what needs localizing.

---

src/L10NSharp/LocalizationManager.cs, line 1782 at r1 (raw file):

      /// directory.
      /// </summary>
      public static void MergeExistingEnglishXliffFileIntoNew(string installedStringFileFolder, string appId)

I'm assuming this long block of code is moved and not substantially changed.

---

src/L10NSharpTests/LocalizationManagerTests.cs, line 646 at r1 (raw file):

          var tu = mergedDoc.GetTransUnitForId("This.test");
          Assert.IsNotNull(tu);

I won't insist on changing this, but for future reference...how do you feel about the new(?) Assert.That? I find it much clearer to write something like Assert.That(tu.Source.Lang, Is.EqualTo("en")), though it's a fraction longer. I'd also encourage you to look for opportunities to make helper assert methods that can make things shorter. For example, with the right helper method, a good deal of this could look something like AssertExpectedTu(id="This.test", sourceLang="en", notesCount=2, secondNote="This is only a test", dynamic=true). (The first note is predictable, right, so you wouldn't need an argument for it?) Or you could have an array of strings called extraNotes.

---

Comments from Reviewable

[StephenMcConnel]

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

src/L10NSharp/LocalizationManager.cs, line 285 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

This expression or its negation occurs a few times and might be worth capturing in a method, especially since I think it has a quite specific purpose...something like "ScanningForCurrentStrings", right? That is, if the language is English and we're ignoring EnglishXliffFiles, we're using chunks of this code in a special way, not to provide localization of strings but to find what needs localizing.

Done.

---

src/L10NSharp/LocalizationManager.cs, line 1782 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

I'm assuming this long block of code is moved and not substantially changed.

The changes are mostly cosmetic changes in the notes added about various states encountered. Notably, I added a note today to record the old string value whenever the source string changes.

---

src/L10NSharpTests/LocalizationManagerTests.cs, line 646 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

I won't insist on changing this, but for future reference...how do you feel about the new(?) Assert.That? I find it much clearer to write something like Assert.That(tu.Source.Lang, Is.EqualTo("en")), though it's a fraction longer. I'd also encourage you to look for opportunities to make helper assert methods that can make things shorter. For example, with the right helper method, a good deal of this could look something like AssertExpectedTu(id="This.test", sourceLang="en", notesCount=2, secondNote="This is only a test", dynamic=true). (The first note is predictable, right, so you wouldn't need an argument for it?) Or you could have an array of strings called extraNotes.

Done. I also added a utility method for creating a test TransUnit object.

---

Comments from Reviewable