slgobinath / SafeEyes

Protect your eyes from eye strain using this simple and beautiful, yet extensible break reminder
http://slgobinath.github.io/SafeEyes/
GNU General Public License v3.0
1.44k stars 160 forks source link

check missing translations in CI #608

Open deltragon opened 1 month ago

deltragon commented 1 month ago

Currently, it is easily possible to add a new translatable string in Python, without also adding it to the .pot file or without adding it to the individual languages' .po files by running update-po.sh. It should be possible to write a github action/extend the existing action to ensure that running xgettext/pygettext.py would not add any new strings to the .pot file, and that update-po.sh would not add any new strings to the .po files. (The existing validate_po.py already uses polib, we could reuse that for this check too.)

This would avoid issues like https://github.com/slgobinath/SafeEyes/issues/569 (fixed by https://github.com/slgobinath/SafeEyes/commit/603e1d744510862de2e981ffa5393d6bea2fae7e) in the future.

deltragon commented 1 month ago

I have already written a gettext script to update the .pot file:

xgettext --language=Python --join-existing --no-wrap -d safeeyes --no-location --omit-header \
-o safeeyes/config/locale/safeeyes.pot -- $(find safeeyes/ -name "*.py")

However, I am not entirely happy with its output yet.

deltragon commented 1 month ago

Also, ideally, this would also be a script that contributors could run before submitting a PR.

deltragon commented 1 month ago

And just after writing this, I had a PR have a conflict because of the translation files, which wasn't very fun to resolve. Maybe it makes more sense to add this only as a check before publishing a new release, eg. when making a PR to the release branch.

deltragon commented 1 month ago

Apparently xgettext also supports Glade as a language - I wasn't aware of that. That should make it possible to translate strings in glade files directly without also setting the labels from python. (Also needs the domain attribute in the xml: https://docs.gtk.org/gtk4/class.Builder.html#gtkbuilder-ui-definitions)

deltragon commented 1 month ago

PR https://github.com/slgobinath/SafeEyes/pull/620 partially addresses this, by ensuring that translations are propagated to the .pot template file. However, to translate them in weblate, they also need to be added to the languages' .po files. Since this is very prone to merge conflicts, I intend to add this in a separate CI job afterwards, which only runs on release.

archisman-panigrahi commented 1 month ago

Also, ideally, this would also be a script that contributors could run before submitting a PR.

What would this script do exactly?

deltragon commented 1 month ago

Also, ideally, this would also be a script that contributors could run before submitting a PR.

What would this script do exactly?

It's what I implemented in #620 with validate_po.py and validate_po.py --extract. I think we should document this, so contributors can run the scripts locally and debug the errors on their own machine, rather in CI. (This is inspired by the fact that I have no clue how the .pot file was updated previously - I assume by hand?)

archisman-panigrahi commented 1 month ago

This is inspired by the fact that I have no clue how the .pot file was updated previously - I assume by hand?

Yes, I did that https://github.com/slgobinath/SafeEyes/commits/master/safeeyes/config/locale/safeeyes.pot

I think we should document this

Please document this in the README. We should also document this in the pull request template

archisman-panigrahi commented 1 month ago

There was a translation commit for Italian and German 5 minutes ago, and the CI is broken again :( https://github.com/slgobinath/SafeEyes/commit/89aab2769fe8e27bce5e38738f054106488d4065

deltragon commented 1 month ago

@archisman-panigrahi That was a merge issue with Weblate - I've fixed it in 32996803ce0060ea2b1dc4d3e5d72909227b1b4f, it shouldn't happen anymore, presumably.

deltragon commented 1 month ago

(Elaborating on the merge issue - it was specifically failing because some placeholders had changed from %d to %(num)d, but the translations were translated before that).