Page MenuHomeWildfire Games

Check translations for sprintf errors
AbandonedPublic

Authored by elexis on Nov 16 2018, 4:07 AM.

Details

Reviewers
Itms
Gallaecio
Trac Tickets
#4250
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.Nov 16 2018, 4:07 AM
Stan added a subscriber: Stan.Nov 16 2018, 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.EditedNov 16 2018, 11:30 AM

Nice ?
We should probably also rename the file itself.

source/tools/i18n/checkTranslationsForSpam.py
98

isn't a presentisn't present?

Silier added a subscriber: Silier.EditedNov 16 2018, 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 }

lyv added a subscriber: lyv.Nov 16 2018, 5:25 PM

Whats in the test plan would be very useful.

Itms added a comment.Nov 19 2018, 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.Nov 20 2018, 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.Nov 21 2018, 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.Nov 21 2018, 2:33 PM

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

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

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.

@Itms if you can spare the time that would be very appreciated.

The thing is that what this revision proposal adds is new semantics, it implements an arbitrary coding conventions policy. So I'd be surprised if Dennis adds such a strict test (at least if it's not configurable).
As mentioned there are legitimate exceptions to the rule where translations can use sprintf arguments that the english string does not use.

Itms added a reviewer: Itms.Jan 4 2019, 11:50 AM

Hey! Yes I'll find the time. I haven't focused much on DevOps during the holidays but I'll be back to that.

(Not so urgent, just curious to know)

elexis updated the Trac tickets for this revision.Jan 8 2019, 10:59 AM
elexis updated the Trac tickets for this revision.
elexis removed a subscriber: lyv.Sep 18 2019, 4:43 PM

Yet another case:
https://wildfiregames.com/forum/index.php?/topic/27034-save-game-eeror/

#: gui/common/tooltips.js:421
#, javascript-format
msgid "Add another worker to speed up the repairs by %(second)s second."
msgid_plural ""
"Add another worker to speed up the repairs by %(second)s seconds."
msgstr[0] "%(hotkey)s: 切换至历史"

Wrong chinese translation, perhaps not the only one, starting rP21907.

Gallaecio accepted this revision.Sep 18 2019, 6:55 PM
This revision is now accepted and ready to land.Sep 18 2019, 6:55 PM
Silier added inline comments.Sep 18 2019, 7:07 PM
source/tools/i18n/checkTranslationsForSpam.py
4

should not be here 2019?

Recap: Gallaecio kindly run this script again to see if there are broken translations again found by this script. There are and they were fixed, thanks (see http://irclogs.wildfiregames.com/2019-09/2019-09-18-QuakeNet-%230ad-dev.log)!

[adrian@pondal i18n]$ python2 checkTranslationsForSpam.py 

    WARNING: Remember to regenerate the POT files with “updateTemplates.py” before you run this script.
    POT files are not in the repository.

Checking public-tutorials.pot
Checking public-templates-buildings.pot
Checking public-simulation-technologies.pot
Checking public-gui-other.pot
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.” “”
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.” “”
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” “”
ko.public-gui-other.po: The sprintf argument %(max)s used by the translation “최소: %(min)s, Max: %(max)s” isn't a present in the template string “Min: %(min)s” “”
Checking public-gui-manual.pot
Checking public-gui-ingame.pot
Checking public-gui-gamesetup.pot
ca.public-gui-gamesetup.po: The sprintf argument %(missatge)s used by the translation “No es pot acollir la partida: %(missatge)s.” isn't a present in the template string “Cannot host game: %(message)s.” “”
ca.public-gui-gamesetup.po: The sprintf argument %(missatge)s used by the translation “No es pot unir la partida: %(missatge)s.” isn't a present in the template string “Cannot join game: %(message)s.” “”
Checking public-gui-userreport.pot
sv.public-gui-userreport.po: Found the “http://feedback.wildfiregames.com/(GDPR” URL in the translation, which does not match any of the URLs in the translation template: http://feedback.wildfiregames.com/
ca.public-gui-userreport.po: Found the “https://trac.wildfiregames.com/wiki/UserDataProtection.” URL in the translation, which does not match any of the URLs in the translation template: https://trac.wildfiregames.com/wiki/UserDataProtection
Checking public-civilizations.pot
Checking public-gui-lobby.pot
ca.public-gui-lobby.po: Found the “https://trac.wildfiregames.com/wiki/UserDataProtection.” URL in the translation, which does not match any of the URLs in the translation template: https://trac.wildfiregames.com/wiki/UserDataProtection
Checking public-maps.pot
Checking public-simulation-auras.pot
Checking public-templates-units.pot
Checking public-simulation-other.pot
Checking public-templates-other.pot
Checking mod-selector.pot
zh_TW.mod-selector.po: Found the “https://trac.wildfiregames.com/wiki/GameDataPaths)” URL in the translation, which does not match any of the URLs in the translation template: https://trac.wildfiregames.com/wiki/GameDataPaths).
pt_PT.mod-selector.po: Found the “https://trac.wildfiregames.com/wiki/GameDataPaths” URL in the translation, which does not match any of the URLs in the translation template: https://trac.wildfiregames.com/wiki/GameDataPaths).
zh.mod-selector.po: Found the “https://trac.wildfiregames.com/wiki/GameDataPaths” URL in the translation, which does not match any of the URLs in the translation template: https://trac.wildfiregames.com/wiki/GameDataPaths).
hu.mod-selector.po: Found the “https://trac.wildfiregames.com/wiki/GameDataPaths)” URL in the translation, which does not match any of the URLs in the translation template: https://trac.wildfiregames.com/wiki/GameDataPaths).
ko.mod-selector.po: Found the “https://trac.wildfiregames.com/wiki/GameDataPaths” URL in the translation, which does not match any of the URLs in the translation template: https://trac.wildfiregames.com/wiki/GameDataPaths).
fa.mod-selector.po: Found the “https://trac.wildfiregames.com/wiki/GameDataPaths)” URL in the translation, which does not match any of the URLs in the translation template: https://trac.wildfiregames.com/wiki/GameDataPaths).
ast.mod-selector.po: Found the “https://trac.wildfiregames.com/wiki/GameDataPaths)” URL in the translation, which does not match any of the URLs in the translation template: https://trac.wildfiregames.com/wiki/GameDataPaths).
Checking engine.pot

Should we get this script merged, to make it easier to use it next time?

Stan added a comment.Nov 2 2019, 10:47 AM

I guess your best bet is to run it yourself :)

Yet another:
https://wildfiregames.com/forum/index.php?/topic/27544-civic-center-unusable-in-french/

#: gui/common/tooltips.js:200
#, javascript-format
msgid "%(projectileCount)s %(projectileName)s"
msgid_plural "%(projectileCount)s %(projectileName)s"
msgstr[0] "1%(nomduprojectile)s 2%(nombredeprojectiles)s"
msgstr[1] "1%(nom du projectile)s 2%(nombre de projectiles)s"

As mentioned by Stan, there is a warning on transifex for such a broken string:

So the question is how to notify people more strongly, especially before packaging a release.

can it be done that only approved translations are pulled from transifex?

Stan added a comment.Jan 28 2020, 5:33 PM

Already the case. Those were approved...

Stan abandoned this revision.May 24 2021, 12:40 PM

Fixed by rP25538.