From fb54c02373b2e3fb80a5394af43426842de2b6b1 Mon Sep 17 00:00:00 2001 From: Ian Maia Date: Mon, 17 Feb 2025 18:18:11 +0100 Subject: [PATCH 1/7] Add fail_on_extra_strings parameter --- .../actions/ios/ios_lint_localizations.rb | 11 ++++++++++- .../helper/ios/ios_l10n_linter_helper.rb | 12 +++++++----- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_lint_localizations.rb b/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_lint_localizations.rb index 22f2ca79f..56b43f014 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_lint_localizations.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_lint_localizations.rb @@ -37,7 +37,8 @@ def self.run_linter(params) helper.run( input_dir: resolve_path(params[:input_dir]), base_lang: params[:base_lang], - only_langs: params[:only_langs] + only_langs: params[:only_langs], + fail_on_extra_strings: params[:fail_on_extra_strings] ) end @@ -183,6 +184,14 @@ def self.available_options default_value: true, type: Boolean ), + FastlaneCore::ConfigItem.new( + key: :fail_on_extra_strings, + env_name: 'FL_IOS_LINT_TRANSLATIONS_FAIL_ON_EXTRA_STRINGS', + description: 'Should we report violations when finding strings in translations that are not present in the base language', + optional: true, + default_value: true, + type: Boolean + ), ] end diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_linter_helper.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_linter_helper.rb index 79ef9186d..911e2079d 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_linter_helper.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_linter_helper.rb @@ -60,11 +60,12 @@ def install_swiftgen! # # @param [String] input_dir The path (ideally absolute) to the directory containing the `.lproj` folders to parse # @param [String] base_lang The code name (i.e the basename of one of the `.lproj` folders) of the locale to use as the baseline + # @param [String] fail_on_extra_strings Whether to fail on extra strings # @return [Hash>] A hash of violations, keyed by language code, whose values are the list of violation messages for that language # - def run(input_dir:, base_lang: DEFAULT_BASE_LANG, only_langs: nil) + def run(input_dir:, base_lang: DEFAULT_BASE_LANG, only_langs: nil, fail_on_extra_strings: true) check_swiftgen_installed || install_swiftgen! - find_diffs(input_dir: input_dir, base_lang: base_lang, only_langs: only_langs) + find_diffs(input_dir: input_dir, base_lang: base_lang, only_langs: only_langs, fail_on_extra_strings: fail_on_extra_strings) end ################## @@ -160,11 +161,12 @@ def placeholder_types_for_keys(dir, lang) # @param [String] input_dir The directory where the `.lproj` folders to scan are located # @param [String] base_lang The base language used as source of truth that all other languages will be compared against # @param [Array] only_langs The list of languages to limit the generation for. Useful to focus only on a couple of issues or just one language + # @param [Boolean] fail_on_extra_strings Whether to fail on extra strings # @return [Hash>] A hash of violations, keyed by language code, whose values are the list of violation messages for that language # # @note The returned Hash contains keys only for locales with violations. Locales parsed but without any violations found will not appear in the resulting hash. # - def find_diffs(input_dir:, base_lang:, only_langs: nil) + def find_diffs(input_dir:, base_lang:, only_langs: nil, fail_on_extra_strings: true) Dir.mktmpdir('a8c-lint-translations-') do |tmpdir| # Run SwiftGen langs = only_langs.nil? ? nil : (only_langs + [base_lang]).uniq @@ -181,8 +183,8 @@ def find_diffs(input_dir:, base_lang:, only_langs: nil) next nil if params_for_lang.nil? || params_for_lang.empty? violations = params_for_lang.map do |key, param_types| - next "`#{key}` was unexpected, as it is not present in the base locale." if params_for_base_lang[key].nil? - next "`#{key}` expected placeholders for #{params_for_base_lang[key]} but found #{param_types} instead." if params_for_base_lang[key] != param_types + next "`#{key}` was unexpected, as it is not present in the base locale." if params_for_base_lang[key].nil? && fail_on_extra_strings + next "`#{key}` expected placeholders for #{params_for_base_lang[key]} but found #{param_types} instead." if !params_for_base_lang[key].nil? && params_for_base_lang[key] != param_types end.compact [lang, violations] unless violations.empty? From f7b346afbc68fd24f037f248f45c4ec260a85741 Mon Sep 17 00:00:00 2001 From: Ian Maia Date: Mon, 17 Feb 2025 18:19:23 +0100 Subject: [PATCH 2/7] Update unit tests with new IosLintLocalizationsAction's fail_on_extra_strings parameter --- spec/ios_lint_localizations_spec.rb | 17 ++++++++++++++++- .../extra-strings-in-translations-error.yaml | 10 ++++++++++ .../extra-strings-in-translations-no-error.yaml | 8 ++++++++ 3 files changed, 34 insertions(+), 1 deletion(-) create mode 100644 spec/test-data/translations/ios_lint_localizations/extra-strings-in-translations-error.yaml create mode 100644 spec/test-data/translations/ios_lint_localizations/extra-strings-in-translations-no-error.yaml diff --git a/spec/ios_lint_localizations_spec.rb b/spec/ios_lint_localizations_spec.rb index b7b755245..03bcb0941 100644 --- a/spec/ios_lint_localizations_spec.rb +++ b/spec/ios_lint_localizations_spec.rb @@ -62,8 +62,11 @@ # @param [Bool|nil] check_duplicate_keys If `nil`, the test will run the action with the default `check_duplicate_keys` parameter value. # If a `Bool` value is given, it will pass that. # Using either `Bool` or `nil` adds some cruft, but lets us validate the action default behavior, so it doesn't change unexpectedly. + # @param [Bool|nil] fail_on_extra_strings If `nil`, the test will run the action with the default `fail_on_extra_strings` parameter value. + # If a `Bool` value is given, it will pass that. + # Controls whether to report violations when finding strings in translations that are not present in the base language. # - def run_l10n_linter_test(data_file:, check_duplicate_keys: nil) + def run_l10n_linter_test(data_file:, check_duplicate_keys: nil, fail_on_extra_strings: nil) # Arrange: Prepare test files test_file = File.join(File.dirname(__FILE__), 'test-data', 'translations', 'ios_lint_localizations', "#{data_file}.yaml") yml = YAML.load_file(test_file) @@ -86,6 +89,7 @@ def run_l10n_linter_test(data_file:, check_duplicate_keys: nil) base_lang: 'en' } parameters[:check_duplicate_keys] = check_duplicate_keys unless check_duplicate_keys.nil? + parameters[:fail_on_extra_strings] = fail_on_extra_strings unless fail_on_extra_strings.nil? result = run_described_fastlane_action(parameters) # Assert @@ -205,5 +209,16 @@ def run_l10n_linter_test(data_file:, check_duplicate_keys: nil) expect(result).to eq({ 'fr' => ['`key3` expected placeholders for [Int] but found [] instead.'] }) end + + it 'fails on extra strings in translations by default' do + run_l10n_linter_test(data_file: 'extra-strings-in-translations-error') + end + + it 'do not report extra strings in translations when fail_on_extra_strings is false' do + run_l10n_linter_test( + data_file: 'extra-strings-in-translations-no-error', + fail_on_extra_strings: false + ) + end end end diff --git a/spec/test-data/translations/ios_lint_localizations/extra-strings-in-translations-error.yaml b/spec/test-data/translations/ios_lint_localizations/extra-strings-in-translations-error.yaml new file mode 100644 index 000000000..d8ff7d8aa --- /dev/null +++ b/spec/test-data/translations/ios_lint_localizations/extra-strings-in-translations-error.yaml @@ -0,0 +1,10 @@ +test_data: + en: + '"base_string" = "This is in the base language";' + fr: + '"base_string" = "This is in French"; + "extra_string" = "This string only exists in French";' + +result: + fr: + - '`extra_string` was unexpected, as it is not present in the base locale.' diff --git a/spec/test-data/translations/ios_lint_localizations/extra-strings-in-translations-no-error.yaml b/spec/test-data/translations/ios_lint_localizations/extra-strings-in-translations-no-error.yaml new file mode 100644 index 000000000..d95f760ac --- /dev/null +++ b/spec/test-data/translations/ios_lint_localizations/extra-strings-in-translations-no-error.yaml @@ -0,0 +1,8 @@ +test_data: + en: + '"base_string" = "This is in the base language";' + fr: + '"base_string" = "This is in French"; + "extra_string" = "This string only exists in French";' + +result: {} From 70a721d753b6e97e2cf57aea9649d446cc100cb8 Mon Sep 17 00:00:00 2001 From: Ian Maia Date: Mon, 17 Feb 2025 18:47:06 +0100 Subject: [PATCH 3/7] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 785af0774..7b3282659 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ ### New Features -_None_ +- The `ios_lint_localizations` action now accepts a `fail_on_extra_strings` parameter (defaulting to `true`) to control whether to report violations when finding strings in translations that are not present in the base language. [#631] ### Bug Fixes From 540a4057d808d5dc32f75bc3b05947aaf8fa92cf Mon Sep 17 00:00:00 2001 From: "Ian G. Maia" Date: Tue, 18 Feb 2025 10:56:16 +0100 Subject: [PATCH 4/7] Apply suggestions from code review Fix typo and add a test for the positive value Co-authored-by: Gio Lodi --- spec/ios_lint_localizations_spec.rb | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/spec/ios_lint_localizations_spec.rb b/spec/ios_lint_localizations_spec.rb index 03bcb0941..af1f89635 100644 --- a/spec/ios_lint_localizations_spec.rb +++ b/spec/ios_lint_localizations_spec.rb @@ -214,11 +214,18 @@ def run_l10n_linter_test(data_file:, check_duplicate_keys: nil, fail_on_extra_st run_l10n_linter_test(data_file: 'extra-strings-in-translations-error') end - it 'do not report extra strings in translations when fail_on_extra_strings is false' do + it 'does not report extra strings in translations when fail_on_extra_strings is false' do run_l10n_linter_test( data_file: 'extra-strings-in-translations-no-error', fail_on_extra_strings: false ) end + + it 'does report extra strings in translations when fail_on_extra_strings is true' do + run_l10n_linter_test( + data_file: 'extra-strings-in-translations-error', + fail_on_extra_strings: true + ) + end end end From e23f85c9f04cb7adbfd11da5b219358377f98ff1 Mon Sep 17 00:00:00 2001 From: "Ian G. Maia" Date: Tue, 18 Feb 2025 16:36:52 +0100 Subject: [PATCH 5/7] Fix multiline YAML test fixtures Co-authored-by: Olivier Halligon --- .../extra-strings-in-translations-error.yaml | 10 +++++----- .../extra-strings-in-translations-no-error.yaml | 10 +++++----- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/spec/test-data/translations/ios_lint_localizations/extra-strings-in-translations-error.yaml b/spec/test-data/translations/ios_lint_localizations/extra-strings-in-translations-error.yaml index d8ff7d8aa..518f34add 100644 --- a/spec/test-data/translations/ios_lint_localizations/extra-strings-in-translations-error.yaml +++ b/spec/test-data/translations/ios_lint_localizations/extra-strings-in-translations-error.yaml @@ -1,9 +1,9 @@ test_data: - en: - '"base_string" = "This is in the base language";' - fr: - '"base_string" = "This is in French"; - "extra_string" = "This string only exists in French";' + en: |- + "base_string" = "This is in the base language"; + fr: |- + "base_string" = "This is in French"; + "extra_string" = "This string only exists in French"; result: fr: diff --git a/spec/test-data/translations/ios_lint_localizations/extra-strings-in-translations-no-error.yaml b/spec/test-data/translations/ios_lint_localizations/extra-strings-in-translations-no-error.yaml index d95f760ac..c03dacfea 100644 --- a/spec/test-data/translations/ios_lint_localizations/extra-strings-in-translations-no-error.yaml +++ b/spec/test-data/translations/ios_lint_localizations/extra-strings-in-translations-no-error.yaml @@ -1,8 +1,8 @@ test_data: - en: - '"base_string" = "This is in the base language";' - fr: - '"base_string" = "This is in French"; - "extra_string" = "This string only exists in French";' + en: |- + "base_string" = "This is in the base language"; + fr: |- + "base_string" = "This is in French"; + "extra_string" = "This string only exists in French"; result: {} From c981041094434d430111a6abc895fcfdaa098ee9 Mon Sep 17 00:00:00 2001 From: Ian Maia Date: Tue, 18 Feb 2025 16:53:23 +0100 Subject: [PATCH 6/7] Use right YARD convention for parameters that can accept multiple types --- spec/ios_lint_localizations_spec.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/ios_lint_localizations_spec.rb b/spec/ios_lint_localizations_spec.rb index af1f89635..bd721c46c 100644 --- a/spec/ios_lint_localizations_spec.rb +++ b/spec/ios_lint_localizations_spec.rb @@ -59,11 +59,11 @@ # Helper function that DRYs the code running each test. # # @param [String] data_file The name, without extension or "test-lint-ios-" prefix, of the YML file containing the test input and expected output. - # @param [Bool|nil] check_duplicate_keys If `nil`, the test will run the action with the default `check_duplicate_keys` parameter value. - # If a `Bool` value is given, it will pass that. - # Using either `Bool` or `nil` adds some cruft, but lets us validate the action default behavior, so it doesn't change unexpectedly. - # @param [Bool|nil] fail_on_extra_strings If `nil`, the test will run the action with the default `fail_on_extra_strings` parameter value. - # If a `Bool` value is given, it will pass that. + # @param [Boolean, nil] check_duplicate_keys If `nil`, the test will run the action with the default `check_duplicate_keys` parameter value. + # If a `Boolean` value is given, it will pass that. + # Using either `Boolean` or `nil` adds some cruft, but lets us validate the action default behavior, so it doesn't change unexpectedly. + # @param [Boolean, nil] fail_on_extra_strings If `nil`, the test will run the action with the default `fail_on_extra_strings` parameter value. + # If a `Boolean` value is given, it will pass that. # Controls whether to report violations when finding strings in translations that are not present in the base language. # def run_l10n_linter_test(data_file:, check_duplicate_keys: nil, fail_on_extra_strings: nil) From e90cd9b4d8234333982493d502dacd5fa3ec09a6 Mon Sep 17 00:00:00 2001 From: Ian Maia Date: Tue, 18 Feb 2025 16:58:27 +0100 Subject: [PATCH 7/7] Rename `fail_on_extra_strings` to `fail_on_strings_not_in_base_language` --- CHANGELOG.md | 2 +- .../actions/ios/ios_lint_localizations.rb | 6 +++--- .../helper/ios/ios_l10n_linter_helper.rb | 12 ++++++------ spec/ios_lint_localizations_spec.rb | 16 ++++++++-------- 4 files changed, 18 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b3282659..4ed0b723e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ ### New Features -- The `ios_lint_localizations` action now accepts a `fail_on_extra_strings` parameter (defaulting to `true`) to control whether to report violations when finding strings in translations that are not present in the base language. [#631] +- The `ios_lint_localizations` action now accepts a `fail_on_strings_not_in_base_language` parameter (defaulting to `true`) to control whether to report violations when finding strings in translations that are not present in the base language. [#631] ### Bug Fixes diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_lint_localizations.rb b/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_lint_localizations.rb index 56b43f014..42b051d55 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_lint_localizations.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_lint_localizations.rb @@ -38,7 +38,7 @@ def self.run_linter(params) input_dir: resolve_path(params[:input_dir]), base_lang: params[:base_lang], only_langs: params[:only_langs], - fail_on_extra_strings: params[:fail_on_extra_strings] + fail_on_strings_not_in_base_language: params[:fail_on_strings_not_in_base_language] ) end @@ -185,8 +185,8 @@ def self.available_options type: Boolean ), FastlaneCore::ConfigItem.new( - key: :fail_on_extra_strings, - env_name: 'FL_IOS_LINT_TRANSLATIONS_FAIL_ON_EXTRA_STRINGS', + key: :fail_on_strings_not_in_base_language, + env_name: 'FL_IOS_LINT_TRANSLATIONS_FAIL_ON_STRINGS_NOT_IN_BASE_LANGUAGE', description: 'Should we report violations when finding strings in translations that are not present in the base language', optional: true, default_value: true, diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_linter_helper.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_linter_helper.rb index 911e2079d..292e697b9 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_linter_helper.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_linter_helper.rb @@ -60,12 +60,12 @@ def install_swiftgen! # # @param [String] input_dir The path (ideally absolute) to the directory containing the `.lproj` folders to parse # @param [String] base_lang The code name (i.e the basename of one of the `.lproj` folders) of the locale to use as the baseline - # @param [String] fail_on_extra_strings Whether to fail on extra strings + # @param [String] fail_on_strings_not_in_base_language Whether to fail on strings not in base language # @return [Hash>] A hash of violations, keyed by language code, whose values are the list of violation messages for that language # - def run(input_dir:, base_lang: DEFAULT_BASE_LANG, only_langs: nil, fail_on_extra_strings: true) + def run(input_dir:, base_lang: DEFAULT_BASE_LANG, only_langs: nil, fail_on_strings_not_in_base_language: true) check_swiftgen_installed || install_swiftgen! - find_diffs(input_dir: input_dir, base_lang: base_lang, only_langs: only_langs, fail_on_extra_strings: fail_on_extra_strings) + find_diffs(input_dir: input_dir, base_lang: base_lang, only_langs: only_langs, fail_on_strings_not_in_base_language: fail_on_strings_not_in_base_language) end ################## @@ -161,12 +161,12 @@ def placeholder_types_for_keys(dir, lang) # @param [String] input_dir The directory where the `.lproj` folders to scan are located # @param [String] base_lang The base language used as source of truth that all other languages will be compared against # @param [Array] only_langs The list of languages to limit the generation for. Useful to focus only on a couple of issues or just one language - # @param [Boolean] fail_on_extra_strings Whether to fail on extra strings + # @param [Boolean] fail_on_strings_not_in_base_language Whether to fail on strings not in base language # @return [Hash>] A hash of violations, keyed by language code, whose values are the list of violation messages for that language # # @note The returned Hash contains keys only for locales with violations. Locales parsed but without any violations found will not appear in the resulting hash. # - def find_diffs(input_dir:, base_lang:, only_langs: nil, fail_on_extra_strings: true) + def find_diffs(input_dir:, base_lang:, only_langs: nil, fail_on_strings_not_in_base_language: true) Dir.mktmpdir('a8c-lint-translations-') do |tmpdir| # Run SwiftGen langs = only_langs.nil? ? nil : (only_langs + [base_lang]).uniq @@ -183,7 +183,7 @@ def find_diffs(input_dir:, base_lang:, only_langs: nil, fail_on_extra_strings: t next nil if params_for_lang.nil? || params_for_lang.empty? violations = params_for_lang.map do |key, param_types| - next "`#{key}` was unexpected, as it is not present in the base locale." if params_for_base_lang[key].nil? && fail_on_extra_strings + next "`#{key}` was unexpected, as it is not present in the base locale." if params_for_base_lang[key].nil? && fail_on_strings_not_in_base_language next "`#{key}` expected placeholders for #{params_for_base_lang[key]} but found #{param_types} instead." if !params_for_base_lang[key].nil? && params_for_base_lang[key] != param_types end.compact diff --git a/spec/ios_lint_localizations_spec.rb b/spec/ios_lint_localizations_spec.rb index bd721c46c..af357ab3a 100644 --- a/spec/ios_lint_localizations_spec.rb +++ b/spec/ios_lint_localizations_spec.rb @@ -62,11 +62,11 @@ # @param [Boolean, nil] check_duplicate_keys If `nil`, the test will run the action with the default `check_duplicate_keys` parameter value. # If a `Boolean` value is given, it will pass that. # Using either `Boolean` or `nil` adds some cruft, but lets us validate the action default behavior, so it doesn't change unexpectedly. - # @param [Boolean, nil] fail_on_extra_strings If `nil`, the test will run the action with the default `fail_on_extra_strings` parameter value. + # @param [Boolean, nil] fail_on_strings_not_in_base_language If `nil`, the test will run the action with the default `fail_on_strings_not_in_base_language` parameter value. # If a `Boolean` value is given, it will pass that. # Controls whether to report violations when finding strings in translations that are not present in the base language. # - def run_l10n_linter_test(data_file:, check_duplicate_keys: nil, fail_on_extra_strings: nil) + def run_l10n_linter_test(data_file:, check_duplicate_keys: nil, fail_on_strings_not_in_base_language: nil) # Arrange: Prepare test files test_file = File.join(File.dirname(__FILE__), 'test-data', 'translations', 'ios_lint_localizations', "#{data_file}.yaml") yml = YAML.load_file(test_file) @@ -89,7 +89,7 @@ def run_l10n_linter_test(data_file:, check_duplicate_keys: nil, fail_on_extra_st base_lang: 'en' } parameters[:check_duplicate_keys] = check_duplicate_keys unless check_duplicate_keys.nil? - parameters[:fail_on_extra_strings] = fail_on_extra_strings unless fail_on_extra_strings.nil? + parameters[:fail_on_strings_not_in_base_language] = fail_on_strings_not_in_base_language unless fail_on_strings_not_in_base_language.nil? result = run_described_fastlane_action(parameters) # Assert @@ -210,21 +210,21 @@ def run_l10n_linter_test(data_file:, check_duplicate_keys: nil, fail_on_extra_st expect(result).to eq({ 'fr' => ['`key3` expected placeholders for [Int] but found [] instead.'] }) end - it 'fails on extra strings in translations by default' do + it 'fails on strings not in base language by default' do run_l10n_linter_test(data_file: 'extra-strings-in-translations-error') end - it 'does not report extra strings in translations when fail_on_extra_strings is false' do + it 'does not report strings not in base language when fail_on_strings_not_in_base_language is false' do run_l10n_linter_test( data_file: 'extra-strings-in-translations-no-error', - fail_on_extra_strings: false + fail_on_strings_not_in_base_language: false ) end - it 'does report extra strings in translations when fail_on_extra_strings is true' do + it 'does report strings not in base language when fail_on_strings_not_in_base_language is true' do run_l10n_linter_test( data_file: 'extra-strings-in-translations-error', - fail_on_extra_strings: true + fail_on_strings_not_in_base_language: true ) end end