-
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 5 commits
ef04613
e1fbe30
efb03cc
e7789a4
b6ac1db
09939a0
d103030
2a7813e
4dad49b
756965b
904e2de
56dd66d
f6c78ff
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/ | ||||||||||||||||||||||||||||
|
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.
Contributor
Author
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. |
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| def self.run(params) | ||||||||||||||||||||||||||||
| res_dir = params[:res_dir] | ||||||||||||||||||||||||||||
| source_paths = [File.join(res_dir, 'values', 'strings.xml')] + params[:additional_source_strings_paths] | ||||||||||||||||||||||||||||
|
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
Contributor
Author
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. Addressed in 904e2de. |
||||||||||||||||||||||||||||
| 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
Contributor
Author
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. Addressed in 904e2de. |
||||||||||||||||||||||||||||
| 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' | ||||||||||||||||||||||||||||
|
oguzkocer marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||||
| 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
+105
to
+114
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.
Contributor
Author
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. |
||||||||||||||||||||||||||||
| 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
+115
to
+127
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.
Contributor
Author
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. |
||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||
| 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 | ||
Uh oh!
There was an error while loading. Please reload this page.