Page MenuHomeWildfire Games

Check translations for sprintf errors
Needs ReviewPublic

Authored by elexis on Fri, Nov 16, 4:07 AM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

Sometimes translators use a wrong sprintf argument name, which then leads to error messages.
Since the error messages do not occur until the string is used (for example a specific tooltips),
the errors go unnoticed until someone notices by coincidence.

For example here https://wildfiregames.com/forum/index.php?/topic/25067-hotkey-does-not-exist/&tab=comments but this wasn't the first time it happened.

This script catches all these errors, but also has one false positive as the entire sprintf syntax is not replicated here.

sk.public-gui-other.po: The sprintf argument %(trained)s used by the translation “Štatistika jednotiek (%(trained)s / %(killed)s / %(captured)s / %(lost)s)” isn't a present in the template string “Siege” “”
zh.public-gui-other.po: The sprintf argument %(hotkey)s used by the translation “%(hotkey)s: 切换至历史” isn't a present in the template string “Add another worker to speed up the repairs by %(second)s second.” “Add another worker to speed up the repairs by %(second)s seconds.”
lv.public-gui-other.po: The sprintf argument %(fps)s used by the translation “KS: %(fps)s” isn't a present in the template string “FPS: %(fps)4s” “”
zh_TW.public-gui-other.po: The sprintf argument %(endWarning)s used by the translation “ %(endWarning)s 您似乎正使用非著色器(shader 一種特定功能)的顯示晶片。這個選項將會在未來的 0 A.D. 版本中移除以便啟用更多的進階圖形特色。我們建議您盡速升級您的顯示卡到較新、相容著色器語言的。” isn't a present in the template string “%(warning)s You appear to be using non-shader (fixed function) graphics. This option will be removed in a future 0 A.D. release, to allow for more advanced graphics features. We advise upgrading your graphics card to a more recent, shader-compatible model.” “”
ar.public-gui-other.po: The sprintf argument %(civ)s used by the translation “%(civ)s :الانتقال لشجرة البنية” isn't a present in the template string “%(hotkey)s: Switch to Structure Tree.” “”

Test Plan

Check whether this script could be run by automatically when pulling translations (rather than only from trac:ReleaseProcessDraft?).

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6455
Build 10692: Vulcan BuildJenkins
Build 10691: arc lint + arc unit

Event Timeline

elexis created this revision.Fri, Nov 16, 4:07 AM
Stan added a subscriber: Stan.Fri, Nov 16, 8:22 AM

Nice work !

source/tools/i18n/checkTranslationsForSpam.py
135

Isn't that the natural Python behavior ?

147

We could use argparse to give a nice interface to users and help maybe.

Imarok added a subscriber: Imarok.EditedFri, Nov 16, 11:30 AM

Nice 👍
We should probably also rename the file itself.

source/tools/i18n/checkTranslationsForSpam.py
98

isn't a presentisn't present?

Angen awarded a token.Fri, Nov 16, 5:12 PM
Angen added a subscriber: Angen.EditedFri, Nov 16, 5:19 PM

The sprintf argument %(trained)s used by the translation “Štatistika jednotiek (%(trained)s / %(killed)s / %(captured)s / %(lost)s)” isn't a present in the template string “Siege” “”

is fixed now

(it have been 2 years there, lol }

smiley added a subscriber: smiley.Fri, Nov 16, 5:25 PM

Whats in the test plan would be very useful.

Itms added a comment.Mon, Nov 19, 10:21 PM

Thank you for the patch! Unfortunately those checks are the same as what is checked by build/jenkins/lint-translations.sh. The thing that is missing from this is a check for broken custom tags (like [font] ones, typically).

I am adding a note to ReleaseProcessDraft so the translation lint script is better advertised.

elexis marked 3 inline comments as done.Tue, Nov 20, 3:53 PM
In D1674#66409, @Itms wrote:

Thank you for the patch! Unfortunately those checks are the same as what is checked by build/jenkins/lint-translations.sh. The thing that is missing from this is a check for broken custom tags (like [font] ones, typically).
I am adding a note to ReleaseProcessDraft so the translation lint script is better advertised.

Case 1: If it does thesame:
Then we should inform modders and maintainers that provide po files for their mod but don't run a Phabricator+Jenkins instance that they can detect these errors using that script.
People looking for l10n support look at source/tools/l10n/, not at build/jenkins/.

Case 2:
Looking at that file, at https://github.com/willkg/dennis and at rP19669, I'm actually not sure that it does the same.
Do you have a link that shows which sprintf test it actually does?
Does it only perform a syntax test, i.e. blame %(foos due to the missing ), or does it also compare the po file against the pot file to test that the foo sprintf argument in the translation is actually a sprintf argument contained in the english string in the pot file?
Secondly, this patch enables WFG to detect all of these errors when running the script, does the linting sh script also provide that (does it not only post feedback on an uploaded patch on Phabricator)?

source/tools/i18n/checkTranslationsForSpam.py
98

📦

135

We don't want an error stack, but a silent shutdown on KeyboardInterrupt

147

Yes

In particular: If we already have a script that satisfies this purpose, why didn't we find the broken sprintf arguments before?

"it have been 2 years there"

Itms added a comment.Wed, Nov 21, 10:41 AM

So the big flaw of rP19669 is that the Phabricator Jenkins plugin has this issue: https://github.com/uber/phabricator-jenkins-plugin/issues/224

After reporting this issue, I left this thing dormant, which caused you to not be aware of the script. I should indeed have looked into making the script usable by more people (which includes giving some pointers about installing Dennis).

In D1674#66415, @elexis wrote:

Case 1: If it does thesame:
Case 2:
Looking at that file, at https://github.com/willkg/dennis and at rP19669, I'm actually not sure that it does the same.

It's mostly case 1, but obviously the checks and output are not 100% identical to what your script does... It just serves the same purpose.

Do you have a link that shows which sprintf test it actually does?

What do you mean?

Does it only perform a syntax test, i.e. blame %(foos due to the missing ), or does it also compare the po file against the pot file to test that the foo sprintf argument in the translation is actually a sprintf argument contained in the english string in the pot file?

The latter. It checks the translation against the original.

Secondly, this patch enables WFG to detect all of these errors when running the script, does the linting sh script also provide that (does it not only post feedback on an uploaded patch on Phabricator)?

Well, if one wants to detect errors manually, they only need to run the script and read the output file which is meant to be sent to Phabricator.

In D1674#66467, @elexis wrote:

In particular: If we already have a script that satisfies this purpose, why didn't we find the broken sprintf arguments before?

As I said the script is useless as long as issue 224 prevents the job from posting comments on autobuilder i18n commits. However, I did use the script to check translations for packaged languages during the A23 first release (and maybe A22, can't remember). So it sounds strange that a string from sk, which was packaged, had an unnoticed bug. It would be good to run dennis on the current SVN to see if this one is detected. If it isn't, that's a bug in Dennis; if it is, the faulty translation is not two years old, or a bug in Dennis has been fixed since May.

In D1674#66477, @Itms wrote:

So the big flaw of rP19669 is that the Phabricator Jenkins plugin has this issue: https://github.com/uber/phabricator-jenkins-plugin/issues/224
The script is useless as long as issue 224 prevents the job from posting comments on autobuilder i18n commits.

Ah, it sounded like it would post on revision proposals. Posting red flags on autobuild commits indeed helps.

In D1674#66415, @elexis wrote:

Does it only perform a syntax test, i.e. blame %(foos due to the missing ), or does it also compare the po file against the pot file to test that the foo sprintf argument in the translation is actually a sprintf argument contained in the english string in the pot file?

The latter. It checks the translation against the original.

Do you have a link that shows which sprintf test it actually does?

What do you mean?

Some primary source that indicates that the claim that the dennis script actually does compare the sprintf arguments of po and pot file, showing that it's certainly not a misconception.
The error message thrown by dennis for this case would also help.

Notice that translations may (in both theory and practice) use sprintf arguments that aren't sprintf arguments in the english string. At least one examples that I recall:

  • layout.js:
// Translation: use %(resourceWithinSentence)s if needed
sprintf(translate("%(resourceFirstWord)s exchanged"), {
	"resourceFirstWord": resourceNameFirstWord(res.code),
	"resourceWithinSentence": resourceNameWithinSentence(res.code)
}),

The english string only offers resourceFirstWord, but the translation may use resourceWithinSentence instead.
Hence it seems more like a policy than a strict syntax error that po and pot sprintf arguments should match (thus allowing doubt that they implemented that check that is not a necessity of the syntax).
(The python patch in this diff prevents this string from being reported as a false positive by using the sprintf arguments provided in the translation comment as a source for sprintf arguments by the translation.)
(The examples in rP19669 only show strings that have a sprintf syntax that dennis didn't recognize, but doesn't provide an example for such a 'sprintf-mismatch' case.)

In D1674#66467, @elexis wrote:

In particular: If we already have a script that satisfies this purpose, why didn't we find the broken sprintf arguments before?

It would be good to run dennis on the current SVN to see if this one is detected. If it isn't, that's a bug in Dennis; if it is, the faulty translation is not two years old, or a bug in Dennis has been fixed since May.

That's a plan! There are 4 strings in total with this ingame error, if I'm not mistaken they were nuked on transifex.

Vulcan added a subscriber: Vulcan.Wed, Nov 21, 2:33 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/789/