Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
Adds a Python-based checker for gettext .po files, registers it as a pre-commit hook, and enforces it via CI.
- Introduce
ci/scripts/poChecker.pyto validate syntax and interpolation consistency. - Register a
checkPohook in.pre-commit-config.yamland configure a GitHub Actions job to run it.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| ci/scripts/poChecker.py | New script to run msgfmt and custom interpolation checks on .po files |
| .pre-commit-config.yaml | Add checkPo hook and include it in the skip list |
| .github/workflows/testAndPublish.yml | Add checkPo job to GitHub Actions workflow |
Comments suppressed due to low confidence (1)
.pre-commit-config.yaml:12
- Removing
checkPofrom the skip list will ensure the hook runs locally by default, matching its enforcement in CI.
skip: [checkPo, scons-source, checkPot, unitTest, licenseCheck, pyrightLocal]
8 tasks
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
seanbudd
commented
Jul 14, 2025
Member
Author
|
There are several fixes needed to happen on Crowdin to fix up syntax issues, however the code for checking for these issues is ready for review |
seanbudd
commented
Jul 14, 2025
seanbudd
commented
Jul 14, 2025
seanbudd
commented
Jul 14, 2025
SaschaCowley
requested changes
Jul 15, 2025
Co-authored-by: Sascha Cowley <16543535+SaschaCowley@users.noreply.github.com>
SaschaCowley
approved these changes
Jul 15, 2025
5 tasks
SaschaCowley
pushed a commit
that referenced
this pull request
Jul 15, 2025
Fixup of #18270 Summary of the issue: We use gettext in l10nUtil now, but it wasn't bundled correctly into NVDA Description of user facing changes: l10nutil is fixed for translators Description of development approach: bundle gettext into nvda distributions Testing strategy: test with nvda build Known issues with pull request: none
seanbudd
added a commit
that referenced
this pull request
Jul 18, 2025
Part of #17878 Follow up to #18270 Summary of the issue: Currently, fetching the latest translations from Crowdin is a manual process. Description of developer facing changes: A GitHub action to update translations is added. This will run at weekly interval, just before we do a regular release. Translation freezes should be designed to line up with this time. This can be run manually. This will only update tracked translations for existing languages. A GitHub action to add new languages is added. This is because we have languages which are started in Crowdin, but not ready for sync/release. This will allow us to track new languages. Description of development approach: The Crowdin GitHub actions caused problems when running in a windows environment. Using the API directly with l10nUtil seemed cleaner. To download all translations you must create a project export bundle and download it directly. Adding this to l10nUtil makes it easier for devs to pull all translations from crowdin manually when required.
5 tasks
seanbudd
added a commit
that referenced
this pull request
Jul 22, 2025
Fixup of #18503 and #18270 Summary of the issue: The regex to check for unnamed string interpolations doesn't support the %% symbol, which escapes %. This leads to certain usages of it raising incorrect interpolation errors. File source/locale/tr/LC_MESSAGES/nvda.po: 1 error Message starting on line 10286 Original: "%s%%" Translated: "%%%s" Error: unnamed percent interpolations differ Expected: unnamed percent interpolations in this order: ['%s'] The regex for missing names for brace interpolations is not working. i.e. "{}" should raise an error without a name like "{name}". This causes issues where the order of the arguments change in the string. e.g. "Character: {}\nReplacement: {}" being translated to "Replacement: {}\nCharacter: {}" will result in the expected interpolation being in the wrong place. These should be fixed in the source .po files to add names to instances of "{}". Description of user facing changes: Adds a more rigourous check for interpolations using brace formats, which will reduce string errors in the UI.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link to issue number:
Closes #18269
Summary of the issue:
On the old translation system, pofile's syntax was checked before merging to NVDA.
The new system doesn't have this, leading to issues like #18252
Description of user facing changes:
Translators will receive warnings / errors with NVDA.po when uploading and downloading translations
Description of developer facing changes:
Add a pre-commit hook which checks the syntax of pofiles.
This will protect commits when pulling translations from crowdin.
Description of development approach:
Modernized an existing script from the old translation server
Testing strategy:
Tested running against NVDA
Known issues with pull request:
There are a number of syntax issues in the current translations.
These need to be fixed before merging in a separate PR.
Code Review Checklist:
@coderabbitai summary