Fix ios_l10n_linter to allow missing translations

The previous implementation used a `diff` of textual outputs from SwiftGen, which worked so far because in the host app projects, locales which were missing translations for some keys were previously replaced by `”key” = “key”` entries (this was back when we assumes that our keys were always going to be the English copies)

But since the recent improvements in our Localization Tooling, this is not the case anymore, and any key that does not have a translation in a given local is now just omitted and not present in that locale’s `Localizable.strings` — just as it has always been intended.

This made the logic of the linter break as it relied on this assumption to make the `diff` it ran on the textual outputs be useful. But now that the `Localizable.strings` files for translations might actually miss a lot of the keys present in the reference locale, we needed to be smarter

This changes the approach from doing a plain textual `diff` of the outputs, to parsing the outputs of SwiftGen into a `Hash<Key, ExpectedParametersList>`, so we can then filter and ignore the keys that are missing in each locale, and only raise violations for keys that are present in both the localization and the reference locale (typically English), in other words, only for keys for which there are actual translations to compare the placeholders for.

Author: AliSoftware

lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_lint_localizations.rb

@@ -28,8 +28,8 @@ def self.run_linter(params)
           only_langs: params[:only_langs]
         )
 
-        violations.each do |lang, diff|
-          UI.error "Inconsistencies found between '#{params[:base_lang]}' and '#{lang}':\n\n#{diff}\n"
+        violations.each do |lang, violations|
+          UI.error "Inconsistencies found between '#{params[:base_lang]}' and '#{lang}':\n\n#{violations.join("\n")}\n"
         end
 
         violations

lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_linter_helper.rb

@@ -58,8 +58,7 @@ 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
-        # @return [Hash<String, String>] A hash whose keys are the language codes (basename of `.lproj` folders) for which violations were found,
-        #         and the values are the output of the `diff` showing these violations.
+        # @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)
           check_swiftgen_installed || install_swiftgen!
@@ -87,7 +86,7 @@ def template_content
           <<~TEMPLATE
             {% macro recursiveBlock table item %}
               {% for string in item.strings %}
-            "{{string.key}}" => [{{string.types|join:","}}]
+            {{string.key}} ==> [{{string.types|join:","}}]
               {% endfor %}
               {% for child in item.children %}
               {% call recursiveBlock table child %}
@@ -139,28 +138,27 @@ def generate_swiftgen_config!(input_dir, output_dir, only_langs: nil)
           return [config_file, langs]
         end
 
-        # Because we use English copy verbatim as key names, some keys are the same except for the upper/lowercase.
-        # We need to sort the output again because SwiftGen only sort case-insensitively so that means keys that are
-        # the same except case might be in swapped order for different outputs
+        # 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
         # @param [String] lang The code for the locale we need to sort the output lines for
+        # @return [Hash<String, String>] A hash whose keys are the strings keys, and corresponding value is a String describing the types expected as parameters.
         #
-        def sort_file_lines!(dir, lang)
+        def placeholder_types_for_keys(dir, lang)
           file = File.join(dir, output_filename(lang))
           return nil unless File.exist?(file)
 
-          sorted_lines = File.readlines(file).sort
-          File.write(file, sorted_lines.join)
-          return file
+          File.readlines(file).map do |line|
+            line.match(/^(.*) ==> (\[[A-Za-z,]*\])$/)&.captures
+          end.compact.to_h
         end
 
         # Prepares the template and config files, then run SwiftGen, run `diff` on each generated output against the baseline, and returns a Hash of the violations found.
         #
         # @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<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
-        # @return [Hash<String, String>] A hash of violations, keyed by language code, whose values are the diff output.
+        # @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.
         #
@@ -172,34 +170,22 @@ def find_diffs(input_dir:, base_lang:, only_langs: nil)
             Action.sh(swiftgen_bin, 'config', 'run', '--config', config_file)
 
             # Run diffs
-            base_file = sort_file_lines!(tmpdir, base_lang)
+            params_for_base_lang = placeholder_types_for_keys(tmpdir, base_lang)
             langs.delete(base_lang)
             return langs.map do |lang|
-              file = sort_file_lines!(tmpdir, lang)
+              params_for_lang = placeholder_types_for_keys(tmpdir, lang)
+
               # 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 file.nil? || only_empty_lines?(file)
-
-              # Compute the diff
-              diff = `diff -U0 "#{base_file}" "#{file}"`
-              # Remove the lines starting with `---`/`+++` which contains the file names (which are temp files we don't want to expose in the final diff to users)
-              # Note: We still keep the `@@ from-file-line-numbers to-file-line-numbers @@` lines to help the user know the index of the key to find it faster,
-              #       and also to serve as a clear visual separator between diff entries in the output.
-              #       Those numbers won't be matching the original `.strings` file line numbers because they are line numbers in the SwiftGen-generated intermediate
-              #       file instead, but they can still give an indication at the index in the list of keys at which this difference is located.
-              diff.gsub!(/^(---|\+\+\+).*\n/, '')
-              diff.empty? ? nil : [lang, diff]
-            end.compact.to_h
-          end
-        end
+              next nil if params_for_lang.nil? || params_for_lang.empty?
 
-        # Returns true if the file only contains empty lines, i.e. lines that only contains whitespace (space, tab, CR, LF)
-        def only_empty_lines?(file)
-          File.open(file) do |f|
-            while (line = f.gets)
-              return false unless line.strip.empty?
-            end
+              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
+              end.compact
+
+              [lang, violations] unless violations.empty?
+            end.compact.to_h
           end
-          return true
         end
       end
     end

spec/test-data/translations/test-lint-ios-no-violations.yaml

@@ -5,6 +5,7 @@ test_data:
     "string_int_pos" = "String %1$@ and Int %2$d";
     "string_int_pos_inv" = "Int %2$d before String %1$@";
     "many_placeholders_mix_pos" = "String %2$@, Float %4$f, Int %1$d, Long %5$li, Precise %3$.3lf";
+    "extra_string_not_translated" = "This string might not have a translation yet in %1$d of our locales.";
     "repeated_placeholder" = "String %1$@, yes, I repeat, that's %1$@ indeed, told you %2$d times.";
   fr: |
     "string_no_pos" = "String %@ here.";

spec/test-data/translations/test-lint-ios-tricky-chars.yaml

@@ -23,17 +23,9 @@ test_data:
     "repeated_placeholder" = "السلسلة   %1$@ ، نعم ، أكرر ، هذه   %1$@ بالفعل ، وليست   %2$d."; // OK
 
 result:
-  fr: |
-    @@ -3 +3 @@
-    -"many_placeholders_mix_pos" => [Int,String,Float,Float,Int]
-    +"many_placeholders_mix_pos" => [String,Float,Float,Int]
-    @@ -5 +5 @@
-    -"string_int_pos" => [String,Int]
-    +"string_int_pos" => [Int]
-    @@ -7 +7 @@
-    -"string_no_pos" => [String]
-    +"string_no_pos" => []
-  ar: |
-    @@ -8 +8 @@
-    -"string_pos" => [String]
-    +"string_pos" => []
+  fr:
+    - "`many_placeholders_mix_pos` expected placeholders for [Int,String,Float,Float,Int] but found [String,Float,Float,Int] instead."
+    - "`string_int_pos` expected placeholders for [String,Int] but found [Int] instead."
+    - "`string_no_pos` expected placeholders for [String] but found [] instead."
+  ar:
+    - "`string_pos` expected placeholders for [String] but found [] instead."

spec/test-data/translations/test-lint-ios-wrong-placeholder-count.yaml

@@ -12,6 +12,7 @@ test_data:
     "string_int_pos" = "String %1$@ and Int %2$d";
     "string_int_pos_inv" = "Int %2$d before String %1$@";
     "many_placeholders_mix_pos" = "String %2$@, Float %4$f, Int %1$d, Long %5$li, Precise %3$.3lf";
+    "unexpected_key_not_present_in_base" = "This key should not exist in the translation given it does not exist in English";
     "repeated_placeholder" = "String %1$@, yes, that's it indeed, %@.";
   it: |
     "string_no_pos" = "String %@ here.";
@@ -21,12 +22,9 @@ test_data:
     "many_placeholders_mix_pos" = "String %2$@, Float %4$f, Int %1$d, Long %5$li, Precise %3$.3lf";
     "repeated_placeholder" = "String %1$@, yes, and not %2%@, I repeat, that's not %2$@.";
 
-result: 
-  fr: |
-    @@ -7 +7 @@
-    -"string_no_pos" => [String]
-    +"string_no_pos" => [String,String]
-  it: |
-    @@ -4 +4 @@
-    -"repeated_placeholder" => [String]
-    +"repeated_placeholder" => [String,String]
+result:
+  fr:
+    - "`string_no_pos` expected placeholders for [String] but found [String,String] instead."
+    - "`unexpected_key_not_present_in_base` was unexpected, as it is not present in the base locale."
+  it:
+    - "`repeated_placeholder` expected placeholders for [String] but found [String,String] instead."

spec/test-data/translations/test-lint-ios-wrong-placeholder-types.yaml

@@ -21,19 +21,10 @@ test_data:
     "many_placeholders_mix_pos" = "String %2$@, Float %4$f, Int %1$d, Long %5$li, Precise %3$.3lf";
     "repeated_placeholder" = "String %1$d, yes, I repeat, that's %1$d indeed, I said %d.";
 
-result: 
-  fr: |
-    @@ -3 +3 @@
-    -"many_placeholders_mix_pos" => [Int,String,Float,Int]
-    +"many_placeholders_mix_pos" => [String,Float,Int,Int]
-    @@ -5 +5 @@
-    -"string_int_pos" => [String,Int]
-    +"string_int_pos" => [Int,String]
-    @@ -7 +7 @@
-    -"string_no_pos" => [String]
-    +"string_no_pos" => [Int]
-
-  it: |
-    @@ -4 +4 @@
-    -"repeated_placeholder" => [String]
-    +"repeated_placeholder" => [Int]
+result:
+  fr:
+    - "`many_placeholders_mix_pos` expected placeholders for [Int,String,Float,Int] but found [String,Float,Int,Int] instead."
+    - "`string_int_pos` expected placeholders for [String,Int] but found [Int,String] instead."
+    - "`string_no_pos` expected placeholders for [String] but found [Int] instead."
+  it:
+    - "`repeated_placeholder` expected placeholders for [String] but found [Int] instead."