search

sillsdev / l10nsharp

A .NET localization library for Windows Forms applications.
3 stars 10 forks source link

Add CheckOrFixXliff program (BL-5409, BL-5415) #37

Closed StephenMcConnel closed 6 years ago

StephenMcConnel commented 6 years ago

This change is Reviewable

andrew-polk commented 6 years ago
:lgtm:

I'm not merging in case our discussion today means more changes. But if that isn't the case, go ahead and merge.

Reviewed 7 of 7 files at r1. Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

andrew-polk commented 6 years ago

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

src/L10NSharp/LocalizedStringCache.cs, line 457 at r2 (raw file):

              value = value.Replace(_ampersandReplacement, "&");

          if (langId != "en")

Seems like we should test at least this new code.

src/L10NSharp/LocalizedStringCache.cs, line 471 at r2 (raw file):

                          return fixedValue;
                      }
                      return null;    // don't use an invalid formatting string!

Wouldn't it be better to return the English?

Comments from Reviewable

StephenMcConnel commented 6 years ago

Review status: 8 of 10 files reviewed at latest revision, 2 unresolved discussions.

src/L10NSharp/LocalizedStringCache.cs, line 457 at r2 (raw file):

Previously, andrew-polk wrote…
Seems like we should test at least this new code.

somehow I knew someone was going to say this... Done.

src/L10NSharp/LocalizedStringCache.cs, line 471 at r2 (raw file):

Previously, andrew-polk wrote…
Wouldn't it be better to return the English?

no, because places it's called do this already if they want English (or some other language) as a fallback

Comments from Reviewable

andrew-polk commented 6 years ago
:lgtm:

Reviewed 2 of 2 files at r3. Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable