Merge pull request #418 from wordpress-mobile/fix/ios_l10n_linting

Improve resilience of `ios_lint_localizations` duplicates check against various file formats

Author: AliSoftware

CHANGELOG.md

@@ -14,7 +14,7 @@ _None_
 
 ### Bug Fixes
 
-_None_
+ - Improve resilience of the `ios_lint_localizations` action to support UTF16 files, and to warn and skip files in XML format when trying to detect duplicate keys on `.strings` files. [#418]
 
 ### Internal Changes
 

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

@@ -2,12 +2,11 @@ module Fastlane
   module Actions
     class IosLintLocalizationsAction < Action
       def self.run(params)
-        violations = Hash.new([])
+        violations = nil
 
         loop do
-          # If we did `violations = self.run...` we'd lose the default value for missing key being `[]` that we set above with `Hash.new`.
-          # We want that default value so that we can use `+=` when adding the duplicate keys violations below.
-          violations = violations.merge(self.run_linter(params))
+          violations = self.run_linter(params)
+          violations.default = [] # Set the default value for when querying a missing key
 
           if params[:check_duplicate_keys]
             find_duplicated_keys(params).each do |language, duplicates|
@@ -49,18 +48,21 @@ def self.report(violations:, base_lang:)
       def self.find_duplicated_keys(params)
         duplicate_keys = {}
 
-        files_to_lint = Dir.chdir(params[:input_dir]) do
-          Dir.glob('*.lproj/Localizable.strings').map do |file|
-            {
-              language: File.basename(File.dirname(file), '.lproj'),
-              path: File.join(params[:input_dir], file)
-            }
-          end
-        end
-
+        files_to_lint = Dir.glob('*.lproj/Localizable.strings', base: params[:input_dir])
         files_to_lint.each do |file|
-          duplicates = Fastlane::Helper::Ios::StringsFileValidationHelper.find_duplicated_keys(file: file[:path])
-          duplicate_keys[file[:language]] = duplicates.map { |key, value| "`#{key}` was found at multiple lines: #{value.join(', ')}" } unless duplicates.empty?
+          language = File.basename(File.dirname(file), '.lproj')
+          path = File.join(params[:input_dir], file)
+
+          file_type = Fastlane::Helper::Ios::L10nHelper.strings_file_type(path: path)
+          if file_type == :text
+            duplicates = Fastlane::Helper::Ios::StringsFileValidationHelper.find_duplicated_keys(file: path)
+            duplicate_keys[language] = duplicates.map { |key, value| "`#{key}` was found at multiple lines: #{value.join(', ')}" } unless duplicates.empty?
+          else
+            UI.important <<~WRONG_FORMAT
+              File `#{path}` is in #{file_type} format, while finding duplicate keys only make sense on files that are in ASCII-plist format.
+              Since your files are in #{file_type} format, you should probably disable the `check_duplicate_keys` option from this `#{self.action_name}` call.
+            WRONG_FORMAT
+          end
         end
 
         duplicate_keys

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

@@ -36,6 +36,28 @@ def self.strings_file_type(path:)
           end
         end
 
+        # Read a file line by line and iterate over it (just like `File.readlines` does),
+        # except that it also detects the encoding used by the file (using the BOM if present) when reading it,
+        # and then convert each line to UTF-8 before yielding it
+        #
+        # This is particularly useful if you need to then use a `RegExp` to match part of the lines you're iterating over,
+        # as the `RegExp` (which will typically be UTF-8) and the string you're matching with it have to use the same encoding
+        # (otherwise we would get a `Encoding::CompatibilityError`)
+        #
+        # @important If you are then using a `RegExp` to match the UTF-8 lines you iterate on,
+        # remember to use the `u` flag on it (`/…/u`) to make it UTF-8-aware too.
+        #
+        # @param [String] file The path to the file to read
+        # @yield each line read from the file, after converting it to the UTF-8 encoding
+        #
+        def self.read_utf8_lines(file)
+          # Be sure to guess file encoding using the Byte-Order-Mark, and fallback to UTF-8 if there's no BOM.
+          File.readlines(file, mode: 'rb:BOM|UTF-8').map do |line|
+            # Ensure the line is re-encoded to UTF-8 regardless of the encoding that was used in the input file
+            line.encode(Encoding::UTF_8)
+          end
+        end
+
         # Merge the content of multiple `.strings` files into a new `.strings` text file.
         #
         # @param [Hash<String, String>] paths The paths of the `.strings` files to merge together, associated with the prefix to prepend to each of their respective keys
@@ -68,11 +90,9 @@ def self.merge_strings(paths:, output_path:)
               all_keys_found += string_keys
 
               tmp_file.write("/* MARK: - #{File.basename(input_file)} */\n\n")
-              # Read line-by-line to reduce memory footprint during content copy; Be sure to guess file encoding using the Byte-Order-Mark.
-              File.readlines(input_file, mode: 'rb:BOM|UTF-8').each do |line|
+              # Read line-by-line to reduce memory footprint during content copy
+              read_utf8_lines(input_file).each do |line|
                 unless prefix.nil? || prefix.empty?
-                  # We need to ensure the line and RegExp are using the same encoding, so we transcode everything to UTF-8.
-                  line.encode!(Encoding::UTF_8)
                   # The `/u` modifier on the RegExps is to make them UTF-8
                   line.gsub!(/^(\s*")/u, "\\1#{prefix}") # Lines starting with a quote are considered to be start of a key; add prefix right after the quote
                   line.gsub!(/^(\s*)([A-Z0-9_]+)(\s*=\s*")/ui, "\\1\"#{prefix}\\2\"\\3") # Lines starting with an identifier followed by a '=' are considered to be an unquoted key (typical in InfoPlist.strings files for example)

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

@@ -11,51 +11,51 @@ class StringsFileValidationHelper
 
         TRANSITIONS = {
           root: {
-            /\s/ => :root,
+            /\s/u => :root,
             '/' => :maybe_comment_start,
             '"' => :in_quoted_key
           },
           maybe_comment_start: {
             '/' => :in_line_comment,
-            /\*/ => :in_block_comment
+            /\*/u => :in_block_comment
           },
           in_line_comment: {
             "\n" => :root,
-            /./ => :in_line_comment
+            /./u => :in_line_comment
           },
           in_block_comment: {
             /\*/ => :maybe_block_comment_end,
-            /./m => :in_block_comment
+            /./mu => :in_block_comment
           },
           maybe_block_comment_end: {
             '/' => :root,
-            /./m => :in_block_comment
+            /./mu => :in_block_comment
           },
           in_quoted_key: {
             '"' => lambda do |state, _|
               state.found_key = state.buffer.string.dup
               state.buffer.string = ''
               :after_quoted_key_before_eq
             end,
-            /./ => lambda do |state, c|
+            /./u => lambda do |state, c|
               state.buffer.write(c)
               :in_quoted_key
             end
           },
           after_quoted_key_before_eq: {
-            /\s/ => :after_quoted_key_before_eq,
+            /\s/u => :after_quoted_key_before_eq,
             '=' => :after_quoted_key_and_eq
           },
           after_quoted_key_and_eq: {
-            /\s/ => :after_quoted_key_and_eq,
+            /\s/u => :after_quoted_key_and_eq,
             '"' => :in_quoted_value
           },
           in_quoted_value: {
             '"' => :after_quoted_value,
-            /./m => :in_quoted_value
+            /./mu => :in_quoted_value
           },
           after_quoted_value: {
-            /\s/ => :after_quoted_value,
+            /\s/u => :after_quoted_value,
             ';' => :root
           }
         }.freeze
@@ -70,7 +70,10 @@ def self.find_duplicated_keys(file:)
 
           state = State.new(context: :root, buffer: StringIO.new, in_escaped_ctx: false, found_key: nil)
 
-          File.readlines(file).each_with_index do |line, line_no|
+          # Using our `each_utf8_line` helper instead of `File.readlines` ensures we can also read files that are
+          # encoded in UTF-16, yet process each of their lines as a UTF-8 string, so that `RegExp#match?` don't throw
+          # an `Encoding::CompatibilityError` exception. (Note how all our `RegExp`s in `TRANSITIONS` have the `u` flag)
+          Fastlane::Helper::Ios::L10nHelper.read_utf8_lines(file).each_with_index do |line, line_no|
             line.chars.each_with_index do |c, col_no|
               # Handle escaped characters at a global level.
               # This is more straightforward than having to account for it in the `TRANSITIONS` table.

spec/ios_lint_localizations_spec.rb

@@ -63,11 +63,10 @@
     #
     def run_l10n_linter_test(data_file:, check_duplicate_keys: nil)
       # Arrange: Prepare test files
-      test_file = File.join(File.dirname(__FILE__), 'test-data', 'translations', "test-lint-ios-#{data_file}.yaml")
+      test_file = File.join(File.dirname(__FILE__), 'test-data', 'translations', 'ios_lint_localizations', "#{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)
@@ -133,6 +132,74 @@ def run_l10n_linter_test(data_file:, check_duplicate_keys: nil)
       run_l10n_linter_test(data_file: 'no-strings')
     end
 
+    it 'allows to retry after manual fix' do
+      # Arrange: Prepare test files
+      valid_content = <<~FIXED_CONTENT
+        "string_placeholder" = "String %@ here.";
+      FIXED_CONTENT
+
+      invalid_content = <<~INVALID_CONTENT
+        "string_placeholder" = "Int %d here.";
+      INVALID_CONTENT
+
+      en_lproj = File.join(@test_data_dir, 'en.lproj')
+      FileUtils.mkdir_p(en_lproj)
+      File.write(File.join(en_lproj, 'Localizable.strings'), valid_content)
+
+      fr_lproj = File.join(@test_data_dir, 'fr.lproj')
+      FileUtils.mkdir_p(fr_lproj)
+      File.write(File.join(fr_lproj, 'Localizable.strings'), invalid_content)
+
+      # Assert: Ask to retry after first failure reported and simulated manual fix in between
+      expect(FastlaneCore::UI).to receive(:error).once
+      expect(FastlaneCore::UI).to receive(:confirm) do
+        # Simulate manual fix between the confirm prompt being asked and replying to it
+        File.write(File.join(fr_lproj, 'Localizable.strings'), valid_content)
+        true
+      end
+
+      # Act
+      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',
+        allow_retry: true
+      )
+
+      # Assert
+      expect(result).to eq({}) # No violations anymore after manual fix and first retry
+    end
+
+    it 'warns if input files are not in ASCII-plist format' do
+      # Arrange: Prepare test files
+      en_lproj = File.join(@test_data_dir, 'en.lproj')
+      ascii_file = File.join(File.dirname(__FILE__), 'test-data', 'translations', 'ios_l10n_helper', 'Localizable-utf16.strings')
+      FileUtils.mkdir_p(en_lproj)
+      File.write(File.join(en_lproj, 'Localizable.strings'), File.read(ascii_file))
+
+      fr_lproj = File.join(@test_data_dir, 'fr.lproj')
+      xml_file = File.join(File.dirname(__FILE__), 'test-data', 'translations', 'ios_l10n_helper', 'xml-format.strings')
+      FileUtils.mkdir_p(fr_lproj)
+      File.write(File.join(fr_lproj, 'Localizable.strings'), File.read(xml_file))
+
+      expected_message = <<~EXPECTED_WARNING
+        File `#{fr_lproj}/Localizable.strings` is in xml format, while finding duplicate keys only make sense on files that are in ASCII-plist format.
+        Since your files are in xml format, you should probably disable the `check_duplicate_keys` option from this `ios_lint_localizations` call.
+      EXPECTED_WARNING
+      expect(FastlaneCore::UI).to receive(:important).with(expected_message)
+
+      # Act
+      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'
+      )
+
+      expect(result).to eq({ 'fr' => ['`key3` expected placeholders for [Int] but found [] instead.'] })
+    end
+
     after(:each) do
       FileUtils.remove_entry @test_data_dir
     end