Update persistence to use Xliff 1.2

[JamesPrabu]

This change is 

[papeh]

Reviewed 2 of 42 files at r1. Review status: 2 of 35 files reviewed at latest revision, 3 unresolved discussions.

---

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

          }

          // Before wasting a bunch of time, make sure we can open the file for writing. .Elements("note")

commented code can be removed

---

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

      private string GetLangIdFromXliffFileName(string fileName)
      {
          fileName = fileName.Substring(0, fileName.Length - kFileExtension.Length);

this looks much better than before

---

src/L10NSharp/LocalizedStringCache.cs, line 272 at r1 (raw file):

      ///// dirty and saved, then true is returned. Otherwise, false is returned.
      ///// </summary>
      ///// ------------------------------------------------------------------------------------

commented method can be removed

---

Comments from Reviewable

[papeh]

Reviewed 39 of 42 files at r1, 3 of 3 files at r2. Review status: all files reviewed at latest revision, 15 unresolved discussions.

---

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

Previously, papeh wrote…

commented code can be removed

but the comment part of the comment should not be removed.

---

src/L10NSharp/XLiffUtils/TargetVariant.cs, line 40 at r2 (raw file):

        {
            get { return _value; }
            set { _value = value; }

is there any reason an auto-implemented { get; set; } property wouldn't work here?

---

src/L10NSharp/XLiffUtils/TransUnit.cs, line 54 at r2 (raw file):

      /// ------------------------------------------------------------------------------------
      [XmlElement("source")]
      public List<TransUnitVariant> Sources { get; set; }

Unless Sources is modified by other classes, Sources will either be empty or contain a single English TUV. Would it make sense to have public TransUnitVariant Source { get; set; } instead? Per the XLIFF documentation, there should also be only one Target.

---

src/L10NSharp/XLiffUtils/TransUnit.cs, line 74 at r2 (raw file):

          get { return _notes; }
          set { _notes = value; }
      }

could be auto-implemented

---

src/L10NSharp/XLiffUtils/TransUnit.cs, line 85 at r2 (raw file):

          get
          {
              return (string.IsNullOrEmpty(Id) && Notes.Count == 0 && (Sources == null || Sources.Count == 0));

need to check Targets?

---

src/L10NSharp/XLiffUtils/TransUnit.cs, line 149 at r2 (raw file):

          if (tuv != null)
          {
              if (langId == "en")

"en" could be stored in a constant

---

src/L10NSharp/XLiffUtils/TransUnitVariant.cs, line 4 at r2 (raw file):

#region // Copyright (c) 2009, SIL International. All Rights Reserved.
// <copyright from='2009' to='2009' company='SIL International'>
//        Copyright (c) 2009, SIL International. All Rights Reserved.

2009-2017

---

src/L10NSharp/XLiffUtils/TransUnitVariant.cs, line 45 at r2 (raw file):

        {
            get { return _value; }
            set { _value = value; }

auto-implement

---

src/L10NSharp/XLiffUtils/XLiffFile.cs, line 35 at r1 (raw file):

Previously, hatton (John Hatton) wrote…

Is the indentation correct here? On github it appears that lines 36 and 37 are indented less than the following lines.

36 and 37 are indented with spaces; the rest of the file with tabs

---

src/L10NSharp/XLiffUtils/XLiffHeader.cs, line 32 at r2 (raw file):

      #region Properties
      protected XLiffNote _note = new XLiffNote();

private?

---

src/L10NSharp/XLiffUtils/XLiffNote.cs, line 29 at r2 (raw file):

      /// ------------------------------------------------------------------------------------
      /// <summary>
      /// Gets or sets the lang.

change "lang" to "type", here and in other comments and variable names in this file

---

src/L10NSharp/XLiffUtils/XLiffXmlSerializationHelper.cs, line 66 at r2 (raw file):

          //{
          //  get { return string.Empty; }
          //}

remove commented code

---

src/L10NSharp/XLiffUtils/XLiffXmlSerializationHelper.cs, line 153 at r2 (raw file):

      public static bool SerializeToFile<T>(string filename, T data)
      {
            //((XLiffDocument)data).File.Body = ((XLiffDocument)data).Body.TransUnits;

commented code

---

src/L10NSharpTests/LocalizationManagerTests.cs, line 120 at r2 (raw file):

              var verAttribute = fileElt == null ? null
                  : fileElt.Attribute("product-version");
              var generatedVersion = verAttribute == null ? null : new Version(verAttribute.Value);

rather than something == null ? null : something.Property, how about

Assert.NotNull(something);
var somethingElse = something.Property

---

Comments from Reviewable

[JamesPrabu]

I changed the code based on John Hatton and Haaso's comments.

[papeh]

Reviewed 4 of 42 files at r1, 11 of 11 files at r3. Review status: all files reviewed at latest revision, 9 unresolved discussions.

---

src/L10NSharp/XLiffUtils/TransUnit.cs, line 54 at r2 (raw file):

Previously, papeh wrote…

Unless `Sources` is modified by other classes, `Sources` will either be empty or contain a single English TUV. Would it make sense to have `public TransUnitVariant Source { get; set; }` instead? Per [the XLIFF documentation](http://docs.oasis-open.org/xliff/v1.2/os/xliff-core.html#trans-unit), there should also be only one Target.

Please update the variable names to Source and Target (singular)

---

src/L10NSharp/XLiffUtils/TransUnitVariant.cs, line 4 at r2 (raw file):

Previously, papeh wrote…

2009-2017

This file, for some reason, has three dates to update :astonished:

---

src/L10NSharp/XLiffUtils/XLiffFile.cs, line 35 at r1 (raw file):

Previously, JamesPrabu wrote…

I adjusted indention for lines 36 and 37. Thank you.

for some reason, I'm still seeing spaces here.

---

src/L10NSharp/XLiffUtils/XLiffNote.cs, line 107 at r3 (raw file):

            var note = new XLiffNote();
            note.Text = text;
            note.Type = lang;

type

---

src/L10NSharpTests/LocalizationManagerTests.cs, line 465 at r3 (raw file):

      }

      static TransUnit MakeTransUnit(string lang, string englishVal, string val, string id, bool dynamic)

To keep related parameters next to each other, how about MakeTransUnit(string lang, string val, string englishVal, string id, bool dynamic) or MakeTransUnit(string id, string englishVal, string lang = null, string val = null, bool dynamic = true)

---

src/L10NSharpTests/LocalizationManagerTests.cs, line 469 at r3 (raw file):

          var sources = new TransUnitVariant { Lang = lang, Value = val };
          var targets = new TransUnitVariant { Lang = lang, Value = val };
          if (englishVal != null)

I think it would be better to require English to have a value and allow the other value to be null. (In all current usages where English is null, the other value is English.)

var source = new TransUnitVariant { Lang = "en", Value = englishVal };
var target = lang == null ? null : new TransUnitVariant { Lang = lang, Value = val };

or similar.

---

Comments from Reviewable

[JamesPrabu]

I applied changes based on Hasso's suggestion.

[papeh]

---

Reviewed 3 of 42 files at r1, 7 of 7 files at r4. Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

src/L10NSharp/XLiffUtils/XLiffFile.cs, line 35 at r1 (raw file):

Previously, papeh wrote…

for some reason, I'm still seeing spaces here.

The blank line between the constructor and these variables can stay.

The first two variables are still indented with spaces (should be tabs), but don't spend more than a minute trying to fix them.

---

src/L10NSharpTests/LocalizationManagerTests.cs, line 448 at r4 (raw file):

  englishDoc.AddTransUnit(MakeTransUnit("en", null, "Title", "SuperClassMethod.TestId", false)); // This is the one ID found in our test code
  englishDoc.AddTransUnit(MakeTransUnit("en", null, "Title", "AnotherContext.AnotherDialog.TestId", true)); // Simulates an 'orphan' that we can't otherwise tell we need.

null, null, "Title", "AnotherContext...

---

Comments from Reviewable

[JamesPrabu]

I applied changes based on Hasso's two suggestions.

[papeh]

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

---

Comments from Reviewable