Add android_prune_orphaned_translations action#734
Conversation
Removes <string>/<string-array>/<plurals> entries from values-*/strings.xml whose key is not declared in the source strings (the res dir's default values/strings.xml, optionally unioned with additional_source_strings_paths). Useful after downloading translations from GlotPress to avoid Lint ExtraTranslation errors from keys removed/renamed since the source was synced.
There was a problem hiding this comment.
Pull request overview
Adds a new Fastlane Android action to remove orphaned resource entries from downloaded values-*/strings.xml locale files, preventing Android Lint ExtraTranslation errors after GlotPress exports include stale keys.
Changes:
- Introduces
android_prune_orphaned_translationsaction that computes valid keys from sourcevalues/strings.xml(+ optional additional sources) and prunes invalid entries from locale files. - Adds RSpec coverage for pruning behavior, including the “additional source strings” overlay case.
- Documents the new action in
CHANGELOG.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_prune_orphaned_translations_action.rb |
Implements the new pruning action using Nokogiri XML parsing/serialization. |
spec/android_prune_orphaned_translations_spec.rb |
Adds unit tests validating pruning behavior and reporting counts. |
CHANGELOG.md |
Adds a Trunk entry describing the new action and its intent. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add explicit `require 'fileutils'` to the spec (matches existing specs and is robust if spec_helper's transitive load changes). Declined the suggested `require 'set'` in the action: Set is autoloaded on Ruby >= 3.2 (the gem's floor) and rubocop's Lint/RedundantRequireStatement would reject it.
| FastlaneCore::ConfigItem.new( | ||
| key: :res_dir, | ||
| description: "Path to the Android project's `res` directory containing the `values-*` locale subdirectories to prune", | ||
| type: String, | ||
| optional: false | ||
| ), |
There was a problem hiding this comment.
Seems like a reasonable suggestion.
| FastlaneCore::ConfigItem.new( | ||
| key: :additional_source_strings_paths, | ||
| description: 'Paths to additional default `strings.xml` files whose keys should also be treated as valid ' \ | ||
| '(e.g. a base module that the pruned `res_dir` overlays at build time)', | ||
| type: Array, | ||
| optional: true, | ||
| default_value: [] | ||
| ), |
There was a problem hiding this comment.
This also seems like a good addition.
| source_paths = [File.join(res_dir, 'values', 'strings.xml')] + params[:additional_source_strings_paths] | ||
| valid_keys = collect_keys(source_paths) | ||
|
|
||
| locale_files = Dir.glob(File.join(res_dir, 'values-*', 'strings.xml')) |
Restrict the glob to Android locale-qualifier directories (language, optional region, or BCP-47 b+ form) so non-locale qualifier dirs like values-night or values-v21 are left untouched. Addresses Copilot review on #734.
Fail with a user-facing message when the res dir's default values/strings.xml or any additional_source_strings_paths entry is missing, instead of a low-level Errno::ENOENT. Addresses Copilot review on #734.
| ### New Features | ||
|
|
||
| _None_ | ||
| - Added `android_prune_orphaned_translations` action: removes `<string>`, `<string-array>` and `<plurals>` entries from `values-*/strings.xml` whose key is not declared in the source strings (the res dir's default `values/strings.xml`, optionally unioned with `additional_source_strings_paths`). Useful after downloading translations from GlotPress to avoid Lint `ExtraTranslation` errors from keys removed/renamed since the source was last synced. |
| LOCALE_VALUES_DIR_REGEX = /\Avalues-(?:b\+[a-zA-Z]+(?:\+[a-zA-Z0-9]+)*|[a-z]{2,3}(?:-r(?:[A-Z]{2}|\d{3}))?)\z/ | ||
|
|
||
| def self.run(params) | ||
| res_dir = params[:res_dir] | ||
| source_paths = [File.join(res_dir, 'values', 'strings.xml')] + params[:additional_source_strings_paths] |
There was a problem hiding this comment.
Nitpick. The relative path values/strings.xml appear a number of times. Have you considered DRYing it? Example:
| LOCALE_VALUES_DIR_REGEX = /\Avalues-(?:b\+[a-zA-Z]+(?:\+[a-zA-Z0-9]+)*|[a-z]{2,3}(?:-r(?:[A-Z]{2}|\d{3}))?)\z/ | |
| def self.run(params) | |
| res_dir = params[:res_dir] | |
| source_paths = [File.join(res_dir, 'values', 'strings.xml')] + params[:additional_source_strings_paths] | |
| LOCALE_VALUES_DIR_REGEX = /\Avalues-(?:b\+[a-zA-Z]+(?:\+[a-zA-Z0-9]+)*|[a-z]{2,3}(?:-r(?:[A-Z]{2}|\d{3}))?)\z/ | |
| DEFAULT_SOURCE_STRINGS_FILE_NAME = 'strings.xml' | |
| DEFAULT_SOURCE_STRINGS_RELATIVE_PATH = File.join('values', DEFAULT_SOURCE_STRINGS_FILE_NAME) | |
| def self.run(params) | |
| res_dir = params[:res_dir] | |
| source_paths = [File.join(res_dir, DEFAULT_SOURCE_STRINGS_RELATIVE_PATH)] + params[:additional_source_strings_paths] |
| end | ||
| valid_keys = collect_keys(source_paths) | ||
|
|
||
| locale_files = Dir.glob(File.join(res_dir, 'values-*', 'strings.xml')).select do |file| |
There was a problem hiding this comment.
Following up on my nitpick above
| locale_files = Dir.glob(File.join(res_dir, 'values-*', 'strings.xml')).select do |file| | |
| locale_files = Dir.glob(File.join(res_dir, 'values-*', DEFAULT_SOURCE_STRINGS_FILE_NAME)).select do |file| |
| ##################################################### | ||
|
|
||
| def self.description | ||
| 'Removes translations whose key is not present in the source strings, to avoid Lint `ExtraTranslation` errors' |
There was a problem hiding this comment.
Nitpick. (At least, I think this is the correct grammar)
| 'Removes translations whose key is not present in the source strings, to avoid Lint `ExtraTranslation` errors' | |
| 'Removes translations whose keys are not present in the source strings, to avoid Lint `ExtraTranslation` errors' |
| ### New Features | ||
|
|
||
| _None_ | ||
| - Added `android_prune_orphaned_translations` action: removes `<string>`, `<string-array>` and `<plurals>` entries from `values-*/strings.xml` whose key is not declared in the source strings (the res dir's default `values/strings.xml`, optionally unioned with `additional_source_strings_paths`). Useful after downloading translations from GlotPress to avoid Lint `ExtraTranslation` errors from keys removed/renamed since the source was last synced. [#734] |
There was a problem hiding this comment.
Formatting nitpick. This is my personal opinion, but I find the long changelog entries AI makes noisy. I find the kind of shorter entries we used to make manually easier to read. Or, rather, I find these AI-generated ones easier to gloss over without taking them in.
| - Added `android_prune_orphaned_translations` action: removes `<string>`, `<string-array>` and `<plurals>` entries from `values-*/strings.xml` whose key is not declared in the source strings (the res dir's default `values/strings.xml`, optionally unioned with `additional_source_strings_paths`). Useful after downloading translations from GlotPress to avoid Lint `ExtraTranslation` errors from keys removed/renamed since the source was last synced. [#734] | |
| - `android_prune_orphaned_translations` action: removes `<string>`, `<string-array>` and `<plurals>` entries from `values-*/strings.xml` whose keys are not declared in the source `values/strings.xml optionally unioned with `additional_source_strings_paths`. [#734] |
mokagio
left a comment
There was a problem hiding this comment.
Looking good. Thanks you for implementing this and your other recent work in the tool. Brought me back to the old days...
I left a few of suggestions and nitpicks. I'm happy for this to land as is and to follow up on those myself if it speeds along adoption in WordPress Android.
| # Matches an Android `values-<qualifier>` directory whose qualifier is a locale (a language code, optional | ||
| # region, or a BCP-47 `b+` form), so non-locale qualifier dirs (e.g. `values-night`, `values-v21`, | ||
| # `values-land`) are left untouched. | ||
| LOCALE_VALUES_DIR_REGEX = /\Avalues-(?:b\+[a-zA-Z]+(?:\+[a-zA-Z0-9]+)*|[a-z]{2,3}(?:-r(?:[A-Z]{2}|\d{3}))?)\z/ |
There was a problem hiding this comment.
My AI noticed that values-car would pass this RegEx despite not being a valid locale folder.
I'm not familiar with how values- work in Android, but I asked for documentation and this came up: https://developer.android.com/guide/topics/resources/providing-resources#UiModeQualifier where car is listed as a qualifier for UI mode.
There's a test showing this limitation at https://github.com/wordpress-mobile/release-toolkit/pull/746/changes#diff-fc5d87ba15967a529eaa6080f6bc0f2313e7133e68f21204d26a76aee6540154R92-R110
I don't consider this a blocker to merging because we know that the action will be used on WordPress Android first, which does not have a car version.
What does it do?
Adds an
android_prune_orphaned_translationsaction that removes orphaned translations from downloaded locale files, so they don't trip Android Lint'sExtraTranslationrule.When translations are downloaded from GlotPress, the export can include keys that are no longer in the app's source strings (removed or renamed since the GlotPress source was last synced).
For every
values-*/strings.xmlunderres_dir, the action removes any<string>,<string-array>or<plurals>whosenameis not declared in the source strings — the res dir's defaultvalues/strings.xml, optionally unioned withadditional_source_strings_paths(for flavors that overlay a base module's resources at build time). It re-serializes with the same options the download uses, so the change is a minimal diff.Checklist before requesting a review
bundle exec rubocopto test for code style violations and recommendations.specs/*_spec.rb) if applicable.bundle exec rspecto run the whole test suite and ensure all your tests pass.CHANGELOG.mdfile to describe your changes under the appropriate existing###subsection of the existing## Trunksection.MIGRATION.mdfile to describe how the changes will affect the migration from the previous major version and what the clients will need to change and consider.