This is a RC proposal for linting translations with Dennis after they are fetched from Transifex.
Details
Details
- Reviewers
fcxSanya • leper - Commits
- rP19669: Add a script to lint the translations with Dennis.
Test the script works.
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/506/ for more details.
Comment Actions
You could maybe use a block and redirect that saving some of the redirections.
Adding some reviewers that did do something related to translations or translation verification to get this tested and get some input. (Would be very nice if we could get the url checks done with this, and possibly also the GUI tag escaping.)
Comment Actions
There is not much I can say here, sorry. Once implemented, I would like to see how it integrates with our CI, how the output is handled and presented, and what types of issues it manages to find.
Comment Actions
List of errors and warnings with descriptions is available here: http://dennis.readthedocs.io/en/latest/linting.html
Example of running on r19307: http://pastebin.com/pzE67PyK
There are some useful reports, like missing character in a variable format:
E101: type missing: %(inactiveString)
165:#: gui/session/menu.js:681 gui/session/menu.js:745
166:#, javascript-format
167:msgid "There is %(inactiveString)s."
168:msgid_plural "There are %(inactiveString)s."
169:msgstr[0] "Bez ez eus un %(inactiveString)."
170:msgstr[1] "Bez ez eus %(inactiveString)."
or a translated variable name:
E201: invalid variables: %(missatge)s
244:#: gui/gamesetup/gamesetup_mp.js:308
245:#, javascript-format
246:msgid "Cannot host game: %(message)s."
247:msgstr "No es pot acollir la partida: %(missatge)s."
but also invalid ones:
W303: different html: "<invalid>" vs. "<ungültig>"
1019:#: gui/locale/locale_advanced.js:92
1020:msgid "<invalid>"
1021:msgstr "<ungültig>"
I believe we don't have HTML in our strings, so we can just disable warnings W303 and W304.
E101: type missing: %(fps)
81:#: gui/common/functions_global_object.js:69
82:#, javascript-format
83:msgid "FPS: %(fps)4s"
84:msgstr "FPS: %(fps)4s"
Dennis validates python format variables, while we are using sprintf.js format ones for JS and standard printf format for C++.
We can't just disable E101, since it's useful in other cases (see an example above), but it produces a lot of these invalid reports making the output hard to read.
I guess we should modify Dennis (using custom rules, plugins or something if they support such things or just by forking it) or find a more flexible solution or write a custom one from scratch.
Comment Actions
We could also use something else than <>, I don't have a strong opinion.
E101: type missing: %(fps)
81:#: gui/common/functions_global_object.js:69
82:#, javascript-format
83:msgid "FPS: %(fps)4s"
84:msgstr "FPS: %(fps)4s"Dennis validates python format variables, while we are using sprintf.js format ones for JS and standard printf format for C++.
We can't just disable E101, since it's useful in other cases (see an example above), but it produces a lot of these invalid reports making the output hard to read.
Oh sorry I forgot to link those:
https://github.com/willkg/dennis/issues/97
https://github.com/willkg/dennis/issues/98
Comment Actions
I disabled linting of the two strings that are not compatible with python-format until Dennis has support for c-format and javascript-format. Note: one needs to regenerate translation templates in order to have the dennis-ignore thing working.
I also followed leper's suggestion.
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/935/ for more details.
| source/gui/scripting/ScriptFunctions.cpp | ||
|---|---|---|
| 832 ↗ | (On Diff #1550) | I'll need to check whether this doesn't cause issues with the translation comment, but I guess it should work. |
Comment Actions
Nice work!
(We might want to do something about that maintenanceTasks.sh script taking ages to revert things (maybe just adding files to a list and doing the update afterwards, or using git (via git-svn) and figuring out if that can deal with parallel add/revert commands.)
| binaries/data/mods/public/gui/locale/locale_advanced.js | ||
|---|---|---|
| 92 ↗ | (On Diff #1550) | We could even state "Invalid locale" here, which might be slightly more helpful for translators than random enclosing characters. |
| build/jenkins/lint-translations.sh | ||
| 19 ↗ | (On Diff #1550) | That's a subshell, not a block, so some things might not be passed to it in the way you want (or act like you'd expect). Since we're using set +e and not set -e here that shouldn't be an issue, however just using a block instead of a subshell would make this nicer and less likely to make someone trip up. (See https://www.gnu.org/software/bash/manual/bash.html#Command-Grouping; or just use {} instead of (); tested this locally by introducing something that'd fail and another echo afterwards and it worked) |
| source/gui/scripting/ScriptFunctions.cpp | ||
| 832 ↗ | (On Diff #1550) | This works, however you will need to manually regenerate the templates and commit engine.pot since the maintenanceTasks.sh script will revert it as that is only a comment change. |