Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

check missing translations in CI #608

Open
deltragon opened this issue Jul 16, 2024 · 11 comments
Open

check missing translations in CI #608

deltragon opened this issue Jul 16, 2024 · 11 comments
Assignees

Comments

@deltragon
Copy link
Collaborator

deltragon commented Jul 16, 2024

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 #569 (fixed by 603e1d7) in the future.

@deltragon deltragon self-assigned this Jul 16, 2024
@deltragon
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@deltragon
Copy link
Collaborator Author

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
Copy link
Collaborator Author

deltragon commented Jul 16, 2024

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
Copy link
Collaborator Author

PR #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
Copy link
Collaborator

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

What would this script do exactly?

@deltragon
Copy link
Collaborator Author

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
Copy link
Collaborator

archisman-panigrahi commented Aug 4, 2024

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
Copy link
Collaborator

There was a translation commit for Italian and German 5 minutes ago, and the CI is broken again :(
89aab27

@deltragon
Copy link
Collaborator Author

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

@deltragon
Copy link
Collaborator Author

(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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants