JohnThomson
commented
7 years ago Review status: 0 of 21 files reviewed at latest revision, 15 unresolved discussions.
src/L10NSharp/LocalizedStringCache.cs, line 246 at r1 (raw file):
xliffDoc.File.HardLineBreakReplacement = s_literalNewline; if (OwningManager != null && OwningManager.Name != null) xliffDoc.File.Original = OwningManager.Name + ".dll";
Not sure how much control we have here, but it seems to me that an xliffDoc's unqualified File ought to be the XML file it lives in; we need a more suitable name for the DLL it provides translations for.
src/L10NSharp/TransUnitUpdater.cs, line 132 at r1 (raw file):
_updated = true; tu.RemoveVariant(tuv); if ((tu.Source == null || tu.Source.Value == null) &&
I think you can now use tu.Source?.Value == null
src/L10NSharp/CodeReader/StringExtractor.cs, line 89 at r1 (raw file):
//Debug.WriteLine(String.Format("DEBUG: StringExtractor.DoExtractingWork() loaded resources for {0}", type.FullName)); } #pragma warning disable CS0168
May be worth commenting what warning you are disabling?
src/L10NSharp/XLiffUtils/TransUnit.cs, line 45 at r1 (raw file):
/// ------------------------------------------------------------------------------------ /// <summary> /// Gets the type of translation unit.
Comment wants update
src/L10NSharp/XLiffUtils/TransUnit.cs, line 53 at r1 (raw file):
/// ------------------------------------------------------------------------------------ /// <summary> /// Gets the source of the translation unit.
Saying this about a property called Source in a class called TransUnit doesn't add much. If it deserves a comment at all, it should indicate what sort of source (a file path? Name of the human translator? Name of a language? Text of what was translated?)
src/L10NSharp/XLiffUtils/XLiffDocument.cs, line 89 at r1 (raw file):
if(isTargetNeeded) return File.Body.TransUnits.Select(tu => tu.Source).Where(s => (s != null && s.Lang != null)).Select(s => s.Lang) .Union(File.Body.TransUnits.Select(tu => tu.Target).Where(t => (t != null && t.Lang != null)).Select(t => t.Lang)).Distinct();
This logic doesn't seem to correspond to "isTargetNeeded". I expected this version to select the languages where language and target are BOTH not-null, not where EITHER is not null. Maybe 'targetWanted' to suggest that we want targets as well as sources? At a higher level, I wonder why this is useful. Seems we're mainly interested in the target languages, and maybe sometimes might also care about the source...if both of those mean what I think (i.e., source is just the English, and target is the actual localization data this xliff is providing). Then there's the issue you discovered yesterday, where code is trying to enumerate languages in one xliff where it should be enumerating languages each represented by a separate xliff...this method may go away altogether? Feel free to just ignore improving this for this PR if it's all going away in the next :-)
src/L10NSharp/XLiffUtils/XLiffFile.cs, line 76 at r1 (raw file):
/// ------------------------------------------------------------------------------------ /// <summary> /// Gets or sets the source language found in the XLiff file.
we seem to be contrasting source and target, so why is the source language property called TargetLang?
src/L10NSharp/XLiffUtils/XLiffFile.cs, line 108 at r1 (raw file):
/// ------------------------------------------------------------------------------------ /// <summary> /// Gets or sets the hard linebreak replacement string.
Is there something more useful to say? For example, is this something that will occur in the output when there's a real hard line break in the file, or something that occurs in the file and will be changed to a hard line break in the strings returned to the client?
src/L10NSharp/XLiffUtils/XLiffNote.cs, line 88 at r1 (raw file):
public bool IsEmpty { get { return (string.IsNullOrEmpty(NoteLang) && string.IsNullOrEmpty(Text)); }
I find it surprising that a Note with no text is non-empty, even if it has a language. (I could imagine that is it empty, or at least useless, if EITHER is missing.)
src/L10NSharp/XLiffUtils/XLiffXmlSerializationHelper.cs, line 117 at r1 (raw file):
using (StringWriter writer = new StringWriter(output)) { XmlSerializerNamespaces nameSpace = new XmlSerializerNamespaces();
Should the name be plural?
src/L10NSharpTests/XLiffUtilsTests.cs, line 27 at r1 (raw file):
[Test] public void TestGetAllVariantLanguagesFound()
Should this be enhanced to verify that it can find more than one of each? (Assuming we still need this method at all once you sort out the deeper logic errors :-)
src/xslt/l10nSharpTmxToXliff1-2.xsl, line 14 at r1 (raw file):
<!-- set language code variables according to the TMX header attributes --> <xsl:variable name="srclang"><xsl:value-of select="/tmx/header/@adminlang"/></xsl:variable> <xsl:variable name="targlang"><xsl:value-of select="/tmx/header/@srclang"/></xsl:variable>
Aaargh! So what tmx calls 'source' is what xliff calls 'target', and xliff calls something else (tmx 'admin') source? No wonder comments are getting confused! Not sure what you can do, but try to be sure that every place a 'Source' property is declared it's clear whether we mean the tmx or xliff sense.
src/xslt/l10nSharpTmxToXliff1-2.xsl, line 30 at r1 (raw file):
<xsl:template match="tmx"> <xsl:element name="file"> <xsl:attribute name="original">CHANGE-ME.dll</xsl:attribute>
Did you mean to change this before merging, or is code going to change this later?
src/xslt/l10nSharpTmxToXliff1-2.xsl, line 34 at r1 (raw file):
<xsl:if test="$srclang != $targlang"> <xsl:attribute name="target-language"><xsl:value-of select="$targlang"/></xsl:attribute> </xsl:if>
These lines seem inconsistent with how you got values for srclang and targlang above.
src/xslt/l10nSharpTmxToXliff1-2.xsl, line 84 at r1 (raw file):
<xsl:if test="not(following-sibling::tuv)"> <xsl:apply-templates select="preceding-sibling::note"/> </xsl:if>
Would it be simpler to just do xsl:apply-templates select ="note" following the tuv select at the parent level?
Comments from Reviewable
StephenMcConnel
hatton
This change is