Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ _None_

### New Features

_None_
- Add an opt-in `allow_omitted_placeholders` option to `ios_lint_localizations` so translations that use only some of the base language placeholders, at the same positions and types, are not reported. [#779]

### Bug Fixes

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ def self.run_linter(params)
input_dir: resolve_path(params[:input_dir]),
base_lang: params[:base_lang],
only_langs: params[:only_langs],
fail_on_strings_not_in_base_language: params[:fail_on_strings_not_in_base_language]
fail_on_strings_not_in_base_language: params[:fail_on_strings_not_in_base_language],
allow_omitted_placeholders: params[:allow_omitted_placeholders]
)
end

Expand Down Expand Up @@ -197,6 +198,15 @@ def self.available_options
default_value: true,
type: Boolean
),
FastlaneCore::ConfigItem.new(
key: :allow_omitted_placeholders,
env_name: 'FL_IOS_LINT_TRANSLATIONS_ALLOW_OMITTED_PLACEHOLDERS',
description: 'Accept translations that use only some of the base language placeholders, as long as each one they use has the same position and type (e.g. "One word" for "%1$d word"). ' \
'Malformed placeholders, extra placeholders and type mismatches are still reported',
optional: true,
default_value: false,
type: Boolean
),
]
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,19 @@ class L10nLinterHelper
DEFAULT_BASE_LANG = 'en'
CONFIG_FILE_NAME = 'swiftgen-stringtypes.yml'

# A printf-style placeholder at the start of a string, capturing its optional `N$` position and its conversion character.
# `*` widths and precisions are deliberately not matched, as they consume an extra argument.
PLACEHOLDER_REGEX = /\A%(?:(\d+)\$)?[-+ #0']*\d*(?:\.\d*)?(?:hh|h|ll|l|q|L|z|t|j)?([@dDiuUxXoOfFeEgGaAcCsSp])/
PLACEHOLDER_TYPES = {
'@' => :object,
'c' => :char, 'C' => :char,
's' => :c_string, 'S' => :c_string,
'p' => :pointer
}.merge(
'dDiuUxXoO'.chars.to_h { |c| [c, :integer] },
'fFeEgGaA'.chars.to_h { |c| [c, :float] }
).freeze

attr_reader :install_path, :version

# @param [String] install_path The path to install SwiftGen to. Usually something like "$PROJECT_DIR/vendor/swiftgen/#{SWIFTGEN_VERSION}".
Expand Down Expand Up @@ -61,11 +74,74 @@ 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_strings_not_in_base_language Whether to fail on strings not in base language
# @param [Boolean] allow_omitted_placeholders Whether to accept translations that use only some of the base language placeholders.
# @return [Hash<String, Array<String>>] 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_strings_not_in_base_language: true)
def run(input_dir:, base_lang: DEFAULT_BASE_LANG, only_langs: nil, fail_on_strings_not_in_base_language: true, allow_omitted_placeholders: false)
check_swiftgen_installed || install_swiftgen!
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)
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,
allow_omitted_placeholders: allow_omitted_placeholders
)
end

# Whether a translation is safe to format with the base string's arguments while using only some of them.
#
# That's the case when every placeholder in the translation exists in the base string, at the same position and with the same type, and the translation has fewer placeholders.
# Unused arguments are ignored at runtime, so this is legitimate when a language doesn't need a value.
#
# For example, the English plural "one" form "%1$ld word" can be translated to Romanian as "Un cuvânt", discarding `%1$ld`,
# because Romanian uses "one" only for exactly 1, and "un" already says it.
#
# This only checks the omission is safe at runtime, not that it's right for the language.
# Russian also has a "one" form, but it covers 1, 21, 31, 101…, so "Одно слово" would wrongly read as one word for 21 words, and this would still accept it.
#
# @param [String] base The string in the base language
# @param [String] translation The translated string
# @return [Boolean] `false` if the translation is blank, or either string has a placeholder this can't parse, like the incomplete `%1$l`
#
def self.only_omits_placeholders?(base:, translation:)
return false if translation.strip.empty?

base_placeholders = placeholders(base)
translation_placeholders = placeholders(translation)
return false if base_placeholders.nil? || translation_placeholders.nil?
return false unless translation_placeholders.size < base_placeholders.size

translation_placeholders.all? { |position, type| base_placeholders[position] == type }
end

# Parses the printf-style placeholders of a string.
#
# @param [String] string The string to parse
# @return [Hash<Integer, Symbol>, nil] The placeholder types keyed by argument position,
# or `nil` if a placeholder is malformed, the string mixes positional and sequential placeholders, or a position is reused with a different type
#
def self.placeholders(string)
placeholders = {}
styles = []
index = 0
while (index = string.index('%', index))
if string[index + 1] == '%'
index += 2
next
end

match = PLACEHOLDER_REGEX.match(string[index..])
return nil if match.nil?

styles << (match[1].nil? ? :sequential : :positional)
position = match[1]&.to_i || styles.count
type = PLACEHOLDER_TYPES[match[2]]
return nil if placeholders.key?(position) && placeholders[position] != type

placeholders[position] = type
index += match[0].length
end
styles.uniq.size > 1 ? nil : placeholders
end

##################
Expand Down Expand Up @@ -141,6 +217,11 @@ def generate_swiftgen_config!(input_dir, output_dir, only_langs: nil)
[config_file, langs]
end

# @return [Hash<String, String>] The raw `Localizable.strings` translations for a locale, keyed by string key
def strings_for(input_dir, lang)
Fastlane::Helper::Ios::L10nHelper.read_strings_file_as_hash(path: File.join(input_dir, "#{lang}.lproj", 'Localizable.strings'))
end

# Returns a Hash mapping the list of expected parameter types for each of the keys based in the %… placeholders found in their `.strings` files
#
# @param [String] dir The temporary directory in which the file to sort lines for is located
Expand All @@ -162,11 +243,12 @@ def placeholder_types_for_keys(dir, lang)
# @param [String] base_lang The base language used as source of truth that all other languages will be compared against
# @param [Array<String>] 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_strings_not_in_base_language Whether to fail on strings not in base language
# @param [Boolean] allow_omitted_placeholders Whether to accept translations that use only some of the base language placeholders
# @return [Hash<String, Array<String>>] 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_strings_not_in_base_language: true)
def find_diffs(input_dir:, base_lang:, only_langs: nil, fail_on_strings_not_in_base_language: true, allow_omitted_placeholders: false)
Dir.mktmpdir('a8c-lint-translations-') do |tmpdir|
# Run SwiftGen
langs = only_langs.nil? ? nil : (only_langs + [base_lang]).uniq
Expand All @@ -176,15 +258,26 @@ def find_diffs(input_dir:, base_lang:, only_langs: nil, fail_on_strings_not_in_b
# Run diffs
params_for_base_lang = placeholder_types_for_keys(tmpdir, base_lang)
langs.delete(base_lang)
base_strings = nil
return langs.map do |lang|
params_for_lang = placeholder_types_for_keys(tmpdir, lang)
lang_strings = nil

# If the lang ends up not having any translation at all (e.g. a `.lproj` without any `.strings` file in it but maybe just a storyboard or assets catalog), ignore it
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_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
next if params_for_base_lang[key].nil? || params_for_base_lang[key] == param_types

# SwiftGen's type lists drop argument positions, so omissions are checked against the raw strings instead.
if allow_omitted_placeholders
base_strings ||= strings_for(input_dir, base_lang)
lang_strings ||= strings_for(input_dir, lang)
next if self.class.only_omits_placeholders?(base: base_strings[key].to_s, translation: lang_strings[key].to_s)
end

"`#{key}` expected placeholders for #{params_for_base_lang[key]} but found #{param_types} instead."
end.compact

[lang, violations] unless violations.empty?
Expand Down
73 changes: 73 additions & 0 deletions spec/ios_l10n_linter_helper_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
# frozen_string_literal: true

require 'spec_helper'

describe Fastlane::Helper::Ios::L10nLinterHelper do
describe '.only_omits_placeholders?' do
def only_omits?(base, translation)
described_class.only_omits_placeholders?(base: base, translation: translation)
end

it 'accepts dropping the only placeholder' do
expect(only_omits?('%1$ld word', 'Un cuvânt')).to be true
end

it 'accepts dropping a trailing positional placeholder' do
expect(only_omits?('Post %1$@ has %2$@ like.', 'Postarea %1$@ are o apreciere.')).to be true
end

it 'accepts dropping a leading or middle positional placeholder when the remaining positions keep their types' do
expect(only_omits?('Post %1$@ has %2$@ likes.', 'O postare are %2$@ aprecieri.')).to be true
expect(only_omits?('%1$@, %2$d, %3$@', '%1$@, %3$@')).to be true
end

it 'accepts dropping a trailing sequential placeholder' do
expect(only_omits?('%@ and %d', 'doar %@')).to be true
end

it 'rejects dropping a leading sequential placeholder, as it shifts the remaining arguments' do
expect(only_omits?('%@ and %d', 'doar %d')).to be false
end

it 'treats length modifiers as the same type' do
expect(only_omits?('%1$ld of %2$ld', '%1$d')).to be true
end

it 'ignores escaped percent signs' do
expect(only_omits?('%1$d%% of %2$d', '%1$d%%')).to be true
end

it 'rejects a placeholder whose type differs from the base' do
expect(only_omits?('%1$d of %2$@', '%1$@')).to be false
end

it 'rejects a placeholder at a position missing from the base' do
expect(only_omits?('%1$d of %2$@', '%3$@')).to be false
end

it 'rejects a translation with as many placeholders as the base' do
expect(only_omits?('%1$d items', '%1$d elemente')).to be false
end

it 'rejects a blank translation' do
expect(only_omits?('%1$@ of %2$@ used', '')).to be false
expect(only_omits?('%1$@ of %2$@ used', ' ')).to be false
end

it 'rejects an incomplete placeholder' do
expect(only_omits?('%1$ld words', '%1$l 个字')).to be false
end

it 'rejects a `*` width, as it consumes an extra argument' do
expect(only_omits?('%1$d and %2$d', '%*d')).to be false
end

it 'rejects mixing positional and sequential placeholders' do
expect(only_omits?('%1$@ %2$d %3$@', '%1$@ %d')).to be false
end

it 'rejects reusing a position with a different type' do
expect(only_omits?('%1$d %2$@ %3$@', '%1$d %1$@')).to be false
end
end
end
17 changes: 16 additions & 1 deletion spec/ios_lint_localizations_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,10 @@
# @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.
# @param [Boolean, nil] allow_omitted_placeholders If `nil`, the test will run the action with the default `allow_omitted_placeholders` parameter value.
# If a `Boolean` value is given, it will pass that.
#
def run_l10n_linter_test(data_file:, check_duplicate_keys: nil, fail_on_strings_not_in_base_language: nil)
def run_l10n_linter_test(data_file:, check_duplicate_keys: nil, fail_on_strings_not_in_base_language: nil, allow_omitted_placeholders: 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)
Expand All @@ -90,6 +92,7 @@ def run_l10n_linter_test(data_file:, check_duplicate_keys: nil, fail_on_strings_
}
parameters[:check_duplicate_keys] = check_duplicate_keys unless check_duplicate_keys.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?
parameters[:allow_omitted_placeholders] = allow_omitted_placeholders unless allow_omitted_placeholders.nil?
result = run_described_fastlane_action(parameters)

# Assert
Expand Down Expand Up @@ -227,6 +230,18 @@ def run_l10n_linter_test(data_file:, check_duplicate_keys: nil, fail_on_strings_
fail_on_strings_not_in_base_language: true
)
end

it 'reports translations that omit placeholders by default' do
run_l10n_linter_test(data_file: 'omitted-placeholders-not-allowed')
end

it 'reports translations that omit placeholders when allow_omitted_placeholders is false' do
run_l10n_linter_test(data_file: 'omitted-placeholders-not-allowed', allow_omitted_placeholders: false)
end

it 'accepts translations that only omit placeholders when allow_omitted_placeholders is true, but still reports malformed, extra, and mismatched ones' do
run_l10n_linter_test(data_file: 'omitted-placeholders-allowed', allow_omitted_placeholders: true)
end
end

# Regression coverage for the unquoted-key/value parsing in `StringsFileValidationHelper`, which used
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
test_data:
en: |
"word_count_one" = "%1$ld word";
"word_count_zero" = "%1$ld words";
"latest_post_likes" = "The latest post %1$@ has received %2$@ like.";
"leading_omitted" = "Post %1$@ has %2$@ likes.";
"middle_omitted" = "A %1$@, B %2$d, C %3$@";
"sequential_trailing_omitted" = "%@ and %d";
"sequential_leading_omitted" = "%@ and %d";
"type_mismatch" = "%1$d items";
"extra_placeholder" = "%1$d items";
"incomplete_placeholder" = "%1$ld words";
"empty_translation" = "%1$@ of %2$@ used";
ro: |
"word_count_one" = "Un cuvânt";
"word_count_zero" = "Niciun cuvânt";
"latest_post_likes" = "Ultimul articol %1$@ a avut o apreciere.";
"leading_omitted" = "Un articol are %2$@ aprecieri.";
"middle_omitted" = "A %1$@, C %3$@";
"sequential_trailing_omitted" = "doar %@";
"sequential_leading_omitted" = "doar %d";
"type_mismatch" = "%1$@ elemente";
"extra_placeholder" = "%1$d elemente %2$@";
"incomplete_placeholder" = "%1$l cuvinte";
"empty_translation" = "";

result:
ro:
- "`empty_translation` expected placeholders for [String,String] but found [] instead."
- "`extra_placeholder` expected placeholders for [Int] but found [Int,String] instead."
- "`incomplete_placeholder` expected placeholders for [Int] but found [] instead."
- "`sequential_leading_omitted` expected placeholders for [String,Int] but found [Int] instead."
- "`type_mismatch` expected placeholders for [Int] but found [String] instead."
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
test_data:
en: |
"word_count_one" = "%1$ld word";
"word_count_zero" = "%1$ld words";
"latest_post_likes" = "The latest post %1$@ has received %2$@ like.";
"leading_omitted" = "Post %1$@ has %2$@ likes.";
"middle_omitted" = "A %1$@, B %2$d, C %3$@";
"sequential_trailing_omitted" = "%@ and %d";
"sequential_leading_omitted" = "%@ and %d";
"type_mismatch" = "%1$d items";
"extra_placeholder" = "%1$d items";
"incomplete_placeholder" = "%1$ld words";
"empty_translation" = "%1$@ of %2$@ used";
ro: |
"word_count_one" = "Un cuvânt";
"word_count_zero" = "Niciun cuvânt";
"latest_post_likes" = "Ultimul articol %1$@ a avut o apreciere.";
"leading_omitted" = "Un articol are %2$@ aprecieri.";
"middle_omitted" = "A %1$@, C %3$@";
"sequential_trailing_omitted" = "doar %@";
"sequential_leading_omitted" = "doar %d";
"type_mismatch" = "%1$@ elemente";
"extra_placeholder" = "%1$d elemente %2$@";
"incomplete_placeholder" = "%1$l cuvinte";
"empty_translation" = "";

result:
ro:
- "`empty_translation` expected placeholders for [String,String] but found [] instead."
- "`extra_placeholder` expected placeholders for [Int] but found [Int,String] instead."
- "`incomplete_placeholder` expected placeholders for [Int] but found [] instead."
- "`latest_post_likes` expected placeholders for [String,String] but found [String] instead."
- "`leading_omitted` expected placeholders for [String,String] but found [String] instead."
- "`middle_omitted` expected placeholders for [String,Int,String] but found [String,String] instead."
- "`sequential_leading_omitted` expected placeholders for [String,Int] but found [Int] instead."
- "`sequential_trailing_omitted` expected placeholders for [String,Int] but found [String] instead."
- "`type_mismatch` expected placeholders for [Int] but found [String] instead."
- "`word_count_one` expected placeholders for [Int] but found [] instead."
- "`word_count_zero` expected placeholders for [Int] but found [] instead."
Loading