Merge pull request #353 from wordpress-mobile/fix-ios-linter

Fix `ios_l10n_linter` to allow missing translations in `.strings` files not being flagged as violations

Author: AliSoftware

CHANGELOG.md

@@ -16,7 +16,7 @@ _None_
 
 ### Bug Fixes
 
-_None_
+* Fix `ios_lint_localizations` action so that it no longer mistakely reports missing keys not yet translated in the other locales' `.strings` as violations. [#353]
 
 ### Internal Changes
 

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

@@ -22,17 +22,17 @@ def self.run_linter(params)
           install_path: resolve_path(params[:install_path]),
           version: params[:version]
         )
-        violations = helper.run(
+        all_violations = helper.run(
           input_dir: resolve_path(params[:input_dir]),
           base_lang: params[:base_lang],
           only_langs: params[:only_langs]
         )
 
-        violations.each do |lang, diff|
-          UI.error "Inconsistencies found between '#{params[:base_lang]}' and '#{lang}':\n\n#{diff}\n"
+        all_violations.each do |lang, lang_violations|
+          UI.error "Inconsistencies found between '#{params[:base_lang]}' and '#{lang}':\n\n#{lang_violations.join("\n")}\n"
         end
 
-        violations
+        all_violations
       end
 
       RETRY_MESSAGE = <<~MSG
@@ -148,15 +148,15 @@ def self.output
       end
 
       def self.return_type
-        :hash_of_strings
+        :hash
       end
 
       def self.return_value
-        'A hash, keyed by language code, whose values are the diff found for said language'
+        'A hash, keyed by language code, whose values are arrays of violations found for that language'
       end
 
       def self.authors
-        ['AliSoftware']
+        ['Automattic']
       end
 
       def self.is_supported?(platform)

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/ios_lint_localizations_spec.rb

@@ -54,6 +54,34 @@
   end
 
   context 'Linter' do
+    def run_l10n_linter_test(data_file)
+      # Arrange: Prepare test files
+      test_file = File.join(File.dirname(__FILE__), 'test-data', 'translations', "test-lint-ios-#{data_file}.yaml")
+      yml = YAML.load_file(test_file)
+
+      files = yml['test_data']
+      FileUtils.mkdir_p(@test_data_dir)
+      files.each do |lang, content|
+        lproj = File.join(@test_data_dir, "#{lang}.lproj")
+        FileUtils.mkdir_p(lproj)
+        File.write(File.join(lproj, 'Localizable.strings'), content) unless content.nil?
+      end
+
+      # Act
+      # Note: We will install SwiftGen in vendor/swiftgen if it's not already installed yet, and intentionally won't
+      #       remove this after the test ends, so that further executions of the test run faster.
+      #       Only the first execution of the tests might take longer if it needs to install SwiftGen first to be able to run the tests.
+      install_dir = "vendor/swiftgen/#{Fastlane::Helper::Ios::L10nLinterHelper::SWIFTGEN_VERSION}"
+      result = run_described_fastlane_action(
+        install_path: install_dir,
+        input_dir: @test_data_dir,
+        base_lang: 'en'
+      )
+
+      # Assert
+      expect(result).to eq(yml['result'])
+    end
+
     before(:each) do
       @test_data_dir = Dir.mktmpdir('a8c-lint-l10n-tests-data-')
       allow(FastlaneCore::UI).to receive(:abort_with_message!)
@@ -84,31 +112,3 @@
     end
   end
 end
-
-def run_l10n_linter_test(data_file)
-  # Arrange: Prepare test files
-  test_file = File.join(File.dirname(__FILE__), 'test-data', 'translations', "test-lint-ios-#{data_file}.yaml")
-  yml = YAML.load_file(test_file)
-
-  files = yml['test_data']
-  FileUtils.mkdir_p(@test_data_dir)
-  files.each do |lang, content|
-    lproj = File.join(@test_data_dir, "#{lang}.lproj")
-    FileUtils.mkdir_p(lproj)
-    File.write(File.join(lproj, 'Localizable.strings'), content) unless content.nil?
-  end
-
-  # Act
-  # Note: We will install SwiftGen in vendor/swiftgen if it's not already installed yet, and intentionally won't
-  #       remove this after the test ends, so that further executions of the test run faster.
-  #       Only the first execution of the tests might take longer if it needs to install SwiftGen first to be able to run the tests.
-  install_dir = "vendor/swiftgen/#{Fastlane::Helper::Ios::L10nLinterHelper::SWIFTGEN_VERSION}"
-  result = run_described_fastlane_action(
-    install_path: install_dir,
-    input_dir: @test_data_dir,
-    base_lang: 'en'
-  )
-
-  # Assert
-  expect(result).to eq(yml['result'])
-end

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

@@ -4,14 +4,17 @@ test_data:
     "string_pos" = "Pos String %1$@ here.";
     "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";
+    "many_placeholders_mix_pos" = "String %2$@, Float %4$f,\n 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.";
     "string_pos" = "Pos String %1$@ here.";
     "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";
+    "many_placeholders_mix_pos" = "String %2$@, Float %4$f,\
+      Int %1$d, \n Long %5$li, Precise %3$.3lf";
     "repeated_placeholder" = "String %1$@, yes, that's it indeed, won't tell you %2$d times.";
   it: |
     "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."