-
Notifications
You must be signed in to change notification settings - Fork 9
Add android_prune_orphaned_translations action
#734
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
base: trunk
Are you sure you want to change the base?
Changes from all commits
ef04613
e1fbe30
efb03cc
e7789a4
b6ac1db
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,126 @@ | ||||||||||||||||||||||||||||
| # frozen_string_literal: true | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| require 'fastlane/action' | ||||||||||||||||||||||||||||
| require 'nokogiri' | ||||||||||||||||||||||||||||
|
mokagio marked this conversation as resolved.
|
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
mokagio marked this conversation as resolved.
|
||||||||||||||||||||||||||||
| module Fastlane | ||||||||||||||||||||||||||||
| module Actions | ||||||||||||||||||||||||||||
| class AndroidPruneOrphanedTranslationsAction < Action | ||||||||||||||||||||||||||||
| # 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/ | ||||||||||||||||||||||||||||
|
Comment on lines
+9
to
+12
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My AI noticed that I'm not familiar with how 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. |
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| def self.run(params) | ||||||||||||||||||||||||||||
| res_dir = params[:res_dir] | ||||||||||||||||||||||||||||
| source_paths = [File.join(res_dir, 'values', 'strings.xml')] + params[:additional_source_strings_paths] | ||||||||||||||||||||||||||||
|
Comment on lines
+12
to
+16
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick. The relative path
Suggested change
|
||||||||||||||||||||||||||||
| source_paths.each do |path| | ||||||||||||||||||||||||||||
| UI.user_error!("Source strings file not found: `#{path}`") unless File.file?(path) | ||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||
| valid_keys = collect_keys(source_paths) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| locale_files = Dir.glob(File.join(res_dir, 'values-*', 'strings.xml')).select do |file| | ||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Following up on my nitpick above
Suggested change
|
||||||||||||||||||||||||||||
| File.basename(File.dirname(file)).match?(LOCALE_VALUES_DIR_REGEX) | ||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||
| total_pruned = 0 | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| locale_files.each do |file| | ||||||||||||||||||||||||||||
| pruned = prune_file(file: file, valid_keys: valid_keys) | ||||||||||||||||||||||||||||
| next if pruned.empty? | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| total_pruned += pruned.count | ||||||||||||||||||||||||||||
| UI.message("Pruned #{pruned.count} orphaned entries from `#{file}`: #{pruned.join(', ')}") | ||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| UI.success("Pruned #{total_pruned} orphaned translation entries across #{locale_files.count} locale file(s).") | ||||||||||||||||||||||||||||
| total_pruned | ||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| # Collects the set of resource names (string, string-array, plurals, …) declared in the given strings files. | ||||||||||||||||||||||||||||
| # | ||||||||||||||||||||||||||||
| # @param [Array<String>] paths The strings.xml files to read the valid keys from. | ||||||||||||||||||||||||||||
| # @return [Set<String>] The set of declared resource names. | ||||||||||||||||||||||||||||
| def self.collect_keys(paths) | ||||||||||||||||||||||||||||
| paths.each_with_object(Set.new) do |path, keys| | ||||||||||||||||||||||||||||
| doc = File.open(path) { |f| Nokogiri::XML(f, nil, Encoding::UTF_8.to_s) } | ||||||||||||||||||||||||||||
| doc.xpath('/resources/*[@name]').each { |node| keys << node['name'] } | ||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| # Removes from `file` any resource entry whose `name` is not in `valid_keys`, preserving the rest of the | ||||||||||||||||||||||||||||
| # file's formatting (so the change shows up as a minimal diff). | ||||||||||||||||||||||||||||
| # | ||||||||||||||||||||||||||||
| # @param [String] file The locale strings.xml file to prune. | ||||||||||||||||||||||||||||
| # @param [Set<String>] valid_keys The set of names that are allowed to remain. | ||||||||||||||||||||||||||||
| # @return [Array<String>] The names of the entries that were pruned. | ||||||||||||||||||||||||||||
| def self.prune_file(file:, valid_keys:) | ||||||||||||||||||||||||||||
| doc = File.open(file) { |f| Nokogiri::XML(f, nil, Encoding::UTF_8.to_s) } | ||||||||||||||||||||||||||||
| orphans = doc.xpath('/resources/*[@name]').reject { |node| valid_keys.include?(node['name']) } | ||||||||||||||||||||||||||||
| return [] if orphans.empty? | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| names = orphans.map { |node| node['name'] } | ||||||||||||||||||||||||||||
| orphans.each do |node| | ||||||||||||||||||||||||||||
| # Drop the indentation/newline text node right before the element too, to avoid leaving a blank line. | ||||||||||||||||||||||||||||
| previous = node.previous_sibling | ||||||||||||||||||||||||||||
| previous.remove if previous&.text? && previous.text.strip.empty? | ||||||||||||||||||||||||||||
| node.remove | ||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| File.open(file, 'w') { |f| doc.write_to(f, encoding: Encoding::UTF_8.to_s, indent: 4) } | ||||||||||||||||||||||||||||
| names | ||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| ##################################################### | ||||||||||||||||||||||||||||
| # @!group Documentation | ||||||||||||||||||||||||||||
| ##################################################### | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| def self.description | ||||||||||||||||||||||||||||
| 'Removes translations whose key is not present in the source strings, to avoid Lint `ExtraTranslation` errors' | ||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick. (At least, I think this is the correct grammar)
Suggested change
|
||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| def self.details | ||||||||||||||||||||||||||||
| <<~DETAILS | ||||||||||||||||||||||||||||
| When downloading translations from GlotPress, the export may include keys that are no longer present in | ||||||||||||||||||||||||||||
| the app's source strings (e.g. removed or renamed since the GlotPress source was last synced). Android | ||||||||||||||||||||||||||||
| Lint flags these orphaned translations as `ExtraTranslation` errors. | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| This action removes — from every `values-*/strings.xml` under `res_dir` — any `<string>`, `<string-array>` | ||||||||||||||||||||||||||||
| or `<plurals>` entry whose `name` is not declared in the default `values/strings.xml` of `res_dir`, | ||||||||||||||||||||||||||||
| optionally unioned with `additional_source_strings_paths` (useful when a product flavor overlays a base | ||||||||||||||||||||||||||||
| module's resources at build time, so the base module's keys are valid too). | ||||||||||||||||||||||||||||
| DETAILS | ||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| def self.available_options | ||||||||||||||||||||||||||||
| [ | ||||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||
|
Comment on lines
+96
to
+101
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: [] | ||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||
|
Comment on lines
+102
to
+109
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This also seems like a good addition. |
||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| def self.return_value | ||||||||||||||||||||||||||||
| 'The total number of orphaned translation entries that were pruned' | ||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| def self.authors | ||||||||||||||||||||||||||||
| ['Automattic'] | ||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| def self.is_supported?(platform) | ||||||||||||||||||||||||||||
| platform == :android | ||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,162 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require 'spec_helper' | ||
| require 'tmpdir' | ||
|
mokagio marked this conversation as resolved.
|
||
| require 'fileutils' | ||
|
|
||
| describe Fastlane::Actions::AndroidPruneOrphanedTranslationsAction do | ||
| # Writes `content` to `path`, creating intermediate directories. | ||
| def write_file(path, content) | ||
| FileUtils.mkdir_p(File.dirname(path)) | ||
| File.write(path, content) | ||
| end | ||
|
|
||
| # A default `values/strings.xml` declaring `hello`, `bye`, an array and a plural. | ||
| let(:default_strings) do | ||
| <<~XML | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <resources> | ||
| <string name="hello">Hello</string> | ||
| <string name="bye">Bye</string> | ||
| <string-array name="planets"> | ||
| <item>Earth</item> | ||
| </string-array> | ||
| <plurals name="items"> | ||
| <item quantity="one">%d item</item> | ||
| <item quantity="other">%d items</item> | ||
| </plurals> | ||
| </resources> | ||
| XML | ||
| end | ||
|
|
||
| it 'removes only the entries whose key is not in the default strings, keeping the rest intact' do | ||
| Dir.mktmpdir do |dir| | ||
| res_dir = File.join(dir, 'res') | ||
| write_file(File.join(res_dir, 'values', 'strings.xml'), default_strings) | ||
| fr_file = File.join(res_dir, 'values-fr', 'strings.xml') | ||
| write_file(fr_file, <<~XML) | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <resources> | ||
| <string name="hello">Bonjour</string> | ||
| <string name="orphan_string">Orphelin</string> | ||
| <string name="bye">Au revoir</string> | ||
| <plurals name="orphan_plural"> | ||
| <item quantity="one">%d truc</item> | ||
| <item quantity="other">%d trucs</item> | ||
| </plurals> | ||
| </resources> | ||
| XML | ||
|
|
||
| pruned = run_described_fastlane_action(res_dir: res_dir) | ||
|
|
||
| expect(pruned).to eq(2) | ||
| content = File.read(fr_file) | ||
| expect(content).to include('name="hello"', 'name="bye"') | ||
| expect(content).not_to include('orphan_string', 'orphan_plural') | ||
| # No blank line left behind where the orphaned <string> was removed. | ||
| expect(content).not_to match(/\n[[:space:]]*\n[[:space:]]*<string name="bye"/) | ||
| end | ||
| end | ||
|
|
||
| it 'leaves non-locale qualifier directories (e.g. values-night) untouched' do | ||
| Dir.mktmpdir do |dir| | ||
| res_dir = File.join(dir, 'res') | ||
| write_file(File.join(res_dir, 'values', 'strings.xml'), default_strings) | ||
| # A non-locale qualifier dir with a key absent from the default must NOT be pruned. | ||
| night_file = File.join(res_dir, 'values-night', 'strings.xml') | ||
| night_content = <<~XML | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <resources> | ||
| <string name="night_only">Night</string> | ||
| </resources> | ||
| XML | ||
| write_file(night_file, night_content) | ||
| # A real locale dir with an orphan, to confirm pruning still happens there. | ||
| fr_file = File.join(res_dir, 'values-fr', 'strings.xml') | ||
| write_file(fr_file, <<~XML) | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <resources> | ||
| <string name="hello">Bonjour</string> | ||
| <string name="orphan_string">Orphelin</string> | ||
| </resources> | ||
| XML | ||
|
|
||
| pruned = run_described_fastlane_action(res_dir: res_dir) | ||
|
|
||
| expect(pruned).to eq(1) | ||
| expect(File.read(night_file)).to eq(night_content) | ||
| expect(File.read(fr_file)).not_to include('orphan_string') | ||
| end | ||
| end | ||
|
|
||
| it 'treats keys from `additional_source_strings_paths` as valid (flavor overlay case)' do | ||
| Dir.mktmpdir do |dir| | ||
| res_dir = File.join(dir, 'res') | ||
| write_file(File.join(res_dir, 'values', 'strings.xml'), <<~XML) | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <resources> | ||
| <string name="flavor_only">Flavor</string> | ||
| </resources> | ||
| XML | ||
| base_strings = File.join(dir, 'base', 'values', 'strings.xml') | ||
| write_file(base_strings, default_strings) | ||
| fr_file = File.join(res_dir, 'values-fr', 'strings.xml') | ||
| write_file(fr_file, <<~XML) | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <resources> | ||
| <string name="flavor_only">Saveur</string> | ||
| <string name="hello">Bonjour</string> | ||
| <string name="orphan_string">Orphelin</string> | ||
| </resources> | ||
| XML | ||
|
|
||
| pruned = run_described_fastlane_action(res_dir: res_dir, additional_source_strings_paths: [base_strings]) | ||
|
|
||
| expect(pruned).to eq(1) | ||
| content = File.read(fr_file) | ||
| expect(content).to include('name="flavor_only"', 'name="hello"') | ||
| expect(content).not_to include('orphan_string') | ||
| end | ||
| end | ||
|
|
||
| it 'does nothing and reports zero when there are no orphaned entries' do | ||
| Dir.mktmpdir do |dir| | ||
| res_dir = File.join(dir, 'res') | ||
| write_file(File.join(res_dir, 'values', 'strings.xml'), default_strings) | ||
| fr_file = File.join(res_dir, 'values-fr', 'strings.xml') | ||
| fr_content = <<~XML | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <resources> | ||
| <string name="hello">Bonjour</string> | ||
| <string name="bye">Au revoir</string> | ||
| </resources> | ||
| XML | ||
| write_file(fr_file, fr_content) | ||
|
|
||
| pruned = run_described_fastlane_action(res_dir: res_dir) | ||
|
|
||
| expect(pruned).to eq(0) | ||
| expect(File.read(fr_file)).to eq(fr_content) | ||
| end | ||
| end | ||
|
|
||
| it 'raises a clear error when the res dir has no default strings file' do | ||
| Dir.mktmpdir do |dir| | ||
| res_dir = File.join(dir, 'res') | ||
| FileUtils.mkdir_p(res_dir) | ||
| expect do | ||
| run_described_fastlane_action(res_dir: res_dir) | ||
| end.to raise_error(FastlaneCore::Interface::FastlaneError, /Source strings file not found/) | ||
| end | ||
| end | ||
|
|
||
| it 'raises a clear error when an additional source strings path is missing' do | ||
| Dir.mktmpdir do |dir| | ||
| res_dir = File.join(dir, 'res') | ||
| write_file(File.join(res_dir, 'values', 'strings.xml'), default_strings) | ||
| expect do | ||
| run_described_fastlane_action(res_dir: res_dir, additional_source_strings_paths: [File.join(dir, 'missing.xml')]) | ||
| end.to raise_error(FastlaneCore::Interface::FastlaneError, /Source strings file not found/) | ||
| end | ||
| end | ||
| end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.