Fix #61 : Fix performance issue and add warnings

For each key, we looped through all the keys already read. This was very unefficient. Now we use a hash table to store the previous keys. Results: - 5 minutes export before optimization - less than 5 seconds after optimization What's more, we added some warnings along the way when we see duplicates, or singular / plural mismatch.
faberNovel · Aug 25, 2021 · 2d3217b · 2d3217b
1 parent d42175f
commit 2d3217b
Show file tree

Hide file tree

Showing 10 changed files with 98 additions and 9 deletions.
diff --git a/lib/ad_localize/mappers/csv_path_to_wording.rb b/lib/ad_localize/mappers/csv_path_to_wording.rb
@@ -5,19 +5,29 @@ def map(csv_path:)
         @headers = CSV.foreach(csv_path).first
         return unless valid?(csv_path: csv_path)
         translations = []
+        existing_key_for_label = {}
 
         CSV.foreach(csv_path, headers: true, skip_blanks: true) do |row|
           row_translations = map_row(row: row, locales: locales)
           next if row_translations.blank?
 
-          existing_keys = translations.map(&:key)
-          new_translations = row_translations.reject do |translation|
-            existing_keys.any? do |key|
-              existing_plural_key = key.label == translation.key.label && key.plural? && translation.key.singular?
-              key.same_as?(key: translation.key) || existing_plural_key
-            end
+          current_key = row_translations.first.key
+
+          current_label = current_key.label
+          existing_key = existing_key_for_label[current_label]
+
+          unless existing_key.nil?
+            existing_plural_key = existing_key.label == current_key.label && existing_key.plural? && current_key.singular?
+            existing_singular_key = existing_key.label == current_key.label && existing_key.singular? && current_key.plural?
+            is_same_key = existing_key.same_as?(key: current_key)
+            LOGGER.warn "A plural value already exist for key '#{current_label}'. Remove duplicates." if existing_plural_key
+            LOGGER.warn "A singular value already exist for key '#{current_label}'. Remove duplicates." if existing_singular_key
+            LOGGER.warn "Some values already exist for key '#{current_label}'. Remove duplicates." if is_same_key
+            next if is_same_key || existing_plural_key || existing_singular_key
           end
-          translations.concat(new_translations)
+
+          existing_key_for_label[current_label] = current_key
+          translations.concat(row_translations)
         end
 
         locale_wordings = translations.group_by(&:locale).map do |locale, group|

diff --git a/test/fixtures/exports_reference_duplicates/.DS_Store b/test/fixtures/exports_reference_duplicates/.DS_Store
diff --git a/test/fixtures/exports_reference_duplicates/en.lproj/Localizable.strings b/test/fixtures/exports_reference_duplicates/en.lproj/Localizable.strings
@@ -0,0 +1,2 @@
+"duplicate" = "Duplicate";
+"singular_before_plural" = "Singular before Plural";
diff --git a/test/fixtures/exports_reference_duplicates/en.lproj/Localizable.stringsdict b/test/fixtures/exports_reference_duplicates/en.lproj/Localizable.stringsdict
@@ -0,0 +1,21 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<plist>
+    <dict>
+        <key>plural_before_singular</key>
+        <dict>
+            <key>NSStringLocalizedFormatKey</key>
+            <string>%#@key@</string>
+            <key>key</key>
+            <dict>
+                <key>NSStringFormatSpecTypeKey</key>
+                <string>NSStringPluralRuleType</string>
+                <key>NSStringFormatValueTypeKey</key>
+                <string>d</string>
+                <key>one</key>
+                <string>Plural before Singular</string>
+                <key>other</key>
+                <string>Plural before Singular</string>
+            </dict>
+        </dict>
+    </dict>
+</plist>
diff --git a/test/fixtures/exports_reference_duplicates/fr.lproj/Localizable.strings b/test/fixtures/exports_reference_duplicates/fr.lproj/Localizable.strings
@@ -0,0 +1,2 @@
+"duplicate" = "Duplicate";
+"singular_before_plural" = "Singular before Plural";
diff --git a/test/fixtures/exports_reference_duplicates/fr.lproj/Localizable.stringsdict b/test/fixtures/exports_reference_duplicates/fr.lproj/Localizable.stringsdict
@@ -0,0 +1,21 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<plist>
+    <dict>
+        <key>plural_before_singular</key>
+        <dict>
+            <key>NSStringLocalizedFormatKey</key>
+            <string>%#@key@</string>
+            <key>key</key>
+            <dict>
+                <key>NSStringFormatSpecTypeKey</key>
+                <string>NSStringPluralRuleType</string>
+                <key>NSStringFormatValueTypeKey</key>
+                <string>d</string>
+                <key>one</key>
+                <string>Plural before Singular</string>
+                <key>other</key>
+                <string>Plural before Singular</string>
+            </dict>
+        </dict>
+    </dict>
+</plist>
diff --git a/test/fixtures/reference.csv b/test/fixtures/reference.csv
@@ -8,7 +8,6 @@
 ,save,Enregistrer,,Save,
 #Plurals,,,,,
 ,assess_rate_trip_voiceover##{one},Attribuer %1$@ étoile,,Rate %1$@ star,
-,assess_rate_trip_voiceover##{one},Attribuer %1$@ étoile,,Rate %1$@ star,
 ,assess_rate_trip_voiceover##{other},Attribuer %1$@ étoiles,Si %1$@<=3 : prononcer également assess_comment_placeholder. Si %1$@>3 : prononcer également assess_optional_comment_placeholder,Rate %1$@ stars,
 #InfoPlist,,,,,
 ,NSCameraUsageDescription,Camera utilisé pour blabla,,Camera used for blabla,

diff --git a/test/fixtures/reference1.csv b/test/fixtures/reference1.csv
@@ -8,7 +8,6 @@
 ,save,Sauvegarder,,Save,
 #Plurals,,,,,
 ,assess_rate_trip_voiceover##{one},Attribuer %1$@ étoile,,Rate %1$@ star,
-,assess_rate_trip_voiceover##{one},Attribuer %1$@ étoile,,Rate %1$@ star,
 ,assess_rate_trip_voiceover##{other},Attribuer %1$@ étoiles,Si %1$@<=3 : prononcer également assess_comment_placeholder. Si %1$@>3 : prononcer également assess_optional_comment_placeholder,Rate %1$@ stars,
 #InfoPlist,,,,,
 ,NSCameraUsageDescription,Camera utilisé pour blabla,,Camera used for blabla,

diff --git a/test/fixtures/reference_duplicates.csv b/test/fixtures/reference_duplicates.csv
@@ -0,0 +1,11 @@
+,key,fr,comment fr,en,comment en
+#Generic actions,,,,,
+,duplicate,Duplicate,,Duplicate,
+,duplicate,Erreur,,Error,
+#Plurals,,,,,
+,plural_before_singular##{one},Plural before Singular,,Plural before Singular,
+,plural_before_singular##{one},Plural before Singular,,Plural before Singular,
+,plural_before_singular##{other},Plural before Singular,,Plural before Singular,
+,plural_before_singular,Erreur,,Error,,
+,singular_before_plural,Singular before Plural,,Singular before Plural,,
+,singular_before_plural##{one},Erreur,,Error,,
diff --git a/test/interactors/execute_export_request_test.rb b/test/interactors/execute_export_request_test.rb
@@ -33,6 +33,30 @@ def teardown
         assert_empty(diff.to_s, "File #{generated_file} do not match reference. Diff: \n\n#{diff}\n")
       end
 
+      test 'it should export ios file with duplicated keys' do
+        # Given
+        csv_file = "test/fixtures/reference_duplicates.csv"
+        reference_dir = "test/fixtures/exports_reference_duplicates"
+        assert(File.exist?(csv_file), "File does not exists #{csv_file}")
+        assert(File.exist?(reference_dir), "File does not exists #{reference_dir}")
+
+        # When
+        export_request = Requests::ExportRequest.new(csv_paths: [csv_file], platforms: 'ios')
+        ExecuteExportRequest.new.call(export_request: export_request)
+
+        # Then
+        ios_files(with_platform_directory: false)
+          .reject { |file| file.include? "InfoPlist.strings" }
+          .each do |file|
+            reference_file = "#{reference_dir}/#{file}"
+            generated_file = "exports/#{file}"
+            assert(File.exist?(reference_file), "File does not exists #{reference_file}")
+            assert(File.exist?(generated_file), "File does not exists #{generated_file}")
+            diff = Diffy::Diff.new(reference_file, generated_file, :source => 'files')
+            assert_empty(diff.to_s, "File #{generated_file} do not match reference. Diff: \n\n#{diff}\n")
+          end
+      end
+
       test 'it should export correct platforms files' do
         # Given
         csv_file = "test/fixtures/reference.csv"