Repository navigation
ci: gate cacti.pot updates on i18n diff instead of regenerating - #35
Merged
Merged
Conversation
The "Verify translation template is up to date" step regenerated locales/po/cacti.pot with locales/build_gettext.sh and compared it (ignoring POT-Creation-Date). That couples the check to an exact gettext/xgettext toolchain and fails on unrelated version drift. Replace it with tests/bin/check-i18n-pot.php, which inspects the pull request diff: if any changed line adds, removes, or edits a Cacti i18n call (__(), __n(), __esc(), ... the same keywords build_gettext.sh feeds xgettext) in a file that feeds the template, then locales/po/cacti.pot must also be part of the pull request. The check fails only when that pot update is missing. The step now runs only on pull_request events and diffs against the PR base SHA, mirroring the existing patch-coverage.php approach.
TheWitness
requested review from
bmfmancini,
browniebraun and
xmacan
and
a balanced review from Copilot
October 5, 2026 16:33
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Multiline translation argument edits can bypass the new check and leave the POT stale.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Replaces gettext regeneration in CI with a diff-based check requiring POT updates when translatable strings change.
Changes:
- Adds a PHP diff analyzer for i18n calls.
- Runs the POT check only for pull requests.
| File | Description |
|---|---|
tests/bin/check-i18n-pot.php |
Detects i18n changes and verifies POT inclusion. |
.github/workflows/plugin-ci-workflow.yml |
Integrates the new pull-request check. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
cigamit
approved these changes
Oct 5, 2026
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.
What
Replaces the
Verify translation template is up to dateCI step. It no longerregenerates
locales/po/cacti.potwithlocales/build_gettext.shand comparesit (ignoring
POT-Creation-Date). Instead it inspects the pull request diff andrequires that
locales/po/cacti.potbe part of the PR whenever the translatablestrings actually change.
Why
The old check coupled CI to an exact gettext/xgettext toolchain: a different
GNU gettext version reorders or re-wraps the template and the step fails even
when no translatable string changed. The real intent is simply "if you changed
i18n strings, regenerate and commit the template".
How
New script
tests/bin/check-i18n-pot.php:tests/bin/patch-coverage.php).calls that
build_gettext.shfeeds toxgettext(
__ __n __x __xn __esc __esc_n __esc_x __esc_xn __date __gettext), limited tothe files that feed the template (
find . -maxdepth 2 -name '*.php').change (the added/removed i18n lines are compared as sets).
locales/po/cacti.potis not in the PR, thestep fails with the list of offending files; otherwise it passes.
The step now runs on
pull_requestevents only.Testing
Validated locally against real branch diffs: adding an
__()call without a potupdate fails; adding it with the pot update passes; a non-i18n change passes; a
pure re-indent of an i18n line passes; deleting a template-feeding PHP file that
contained
__()without a pot update fails.