Skip to content

Add a pofile syntax checker#18270

Merged
seanbudd merged 21 commits intobetafrom
addPoFileChecker
Jul 15, 2025
Merged

Add a pofile syntax checker#18270
seanbudd merged 21 commits intobetafrom
addPoFileChecker

Conversation

@seanbudd
Copy link
Copy Markdown
Member

@seanbudd seanbudd commented Jun 17, 2025

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:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Security precautions taken.

@coderabbitai summary

@seanbudd seanbudd requested a review from a team as a code owner June 17, 2025 08:00
@seanbudd seanbudd requested a review from SaschaCowley June 17, 2025 08:00
@seanbudd seanbudd marked this pull request as draft June 18, 2025 01:34
@SaschaCowley SaschaCowley added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Jun 24, 2025
@seanbudd seanbudd added this to the 2025.2 milestone Jul 14, 2025
@seanbudd seanbudd changed the base branch from master to beta July 14, 2025 03:20
@seanbudd seanbudd marked this pull request as ready for review July 14, 2025 03:20
Copilot AI review requested due to automatic review settings July 14, 2025 03:20
@seanbudd seanbudd closed this Jul 14, 2025
@seanbudd seanbudd reopened this Jul 14, 2025
@seanbudd seanbudd marked this pull request as draft July 14, 2025 03:21
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.py to validate syntax and interpolation consistency.
  • Register a checkPo hook in .pre-commit-config.yaml and 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 checkPo from the skip list will ensure the hook runs locally by default, matching its enforcement in CI.
  skip: [checkPo, scons-source, checkPot, unitTest, licenseCheck, pyrightLocal]

@seanbudd seanbudd mentioned this pull request Jul 14, 2025
8 tasks
seanbudd and others added 3 commits July 14, 2025 13:32
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@seanbudd seanbudd marked this pull request as ready for review July 14, 2025 03:46
@seanbudd
Copy link
Copy Markdown
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

Co-authored-by: Sascha Cowley <16543535+SaschaCowley@users.noreply.github.com>
@seanbudd seanbudd requested a review from SaschaCowley July 15, 2025 02:32
@seanbudd seanbudd merged commit 81d43ed into beta Jul 15, 2025
17 of 19 checks passed
@seanbudd seanbudd deleted the addPoFileChecker branch July 15, 2025 02:54
@seanbudd seanbudd mentioned this pull request 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.
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blocked/needs-external-fix conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Validate translations before adding to NVDA

3 participants