Merge pull request #46 from Aeliot-Tm/sorting

Refactor sorting to avoid copy-paste and double sorting
Aeliot-Tm · May 6, 2022 · 88f64ef · 88f64ef
2 parents 6a319ee + 0c8a9c3
commit 88f64ef
Show file tree

Hide file tree

Showing 26 changed files with 136 additions and 112 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -17,9 +17,14 @@ CHANGELOG
     Use `missed_keys: { insert_position: '' }` instead of it.
   * Mark configuration `yaml: { key_pattern: '' }` deprecated.
     Use `linter: { key_valid_pattern: '' }` instead of it.
+  * Mark method `\Aeliot\Bundle\TransMaintain\Service\Yaml\FilesFinder::getLocales()` as deprecated.
+    Use `\Aeliot\Bundle\TransMaintain\Service\LocalesDetector::getLocales()` instead of it.
+  * Mark method `\Aeliot\Bundle\TransMaintain\Service\Yaml\KeysParser::parseFiles()` as deprecated.
+    Use `\Aeliot\Bundle\TransMaintain\Service\Yaml\FileToSingleLevelArrayParser::parseFiles()` instead of it.
   * Refactored gluing of yaml tree to single level array.
   * Refactored rendering of linters' reports.
   * Rename class `\Aeliot\Bundle\TransMaintain\Model\CsvReader` to `\Aeliot\Bundle\TransMaintain\Model\CSV`.
+  * Sort translations files map.
   * Switch name and alias of YAML lint command.
   * Updated package "symfony/translation" version in dev dependencies.
 * Bug fixes:

diff --git a/src/Command/MissedTranslationsLoggedCommand.php b/src/Command/MissedTranslationsLoggedCommand.php
@@ -49,12 +49,8 @@ protected function execute(InputInterface $input, OutputInterface $output): int
         $hasProblems = false;
         $bag = $this->createReportBag();
 
-        $domainsFiles = $this->fileMapBuilder->buildFilesMap($this->getFiles());
-        ksort($domainsFiles);
-
-        foreach ($domainsFiles as $domain => $localesFiles) {
+        foreach ($this->fileMapBuilder->buildFilesMap($this->getFiles()) as $domain => $localesFiles) {
             $locales = array_keys($localesFiles);
-            sort($locales);
             $bag->addLine($domain, $locales);
             $hasProblems = true;
         }

diff --git a/src/Service/FileMapBuilder.php b/src/Service/FileMapBuilder.php
@@ -14,6 +14,16 @@ final class FileMapBuilder
      * @return array<string,array<string,array<int,string>>>
      */
     public function buildFilesMap(iterable $files): array
+    {
+        return $this->sort($this->buildMap($files));
+    }
+
+    /**
+     * @param iterable<SplFileInfo> $files
+     *
+     * @return array<string,array<string,array<int,string>>>
+     */
+    private function buildMap(iterable $files): array
     {
         $map = [];
         foreach ($files as $file) {
@@ -31,4 +41,15 @@ public function buildFilesMap(iterable $files): array
 
         return $map;
     }
+
+    private function sort(array $map): array
+    {
+        ksort($map);
+        foreach ($map as $domain => $localesFiles) {
+            ksort($localesFiles);
+            $map[$domain] = $localesFiles;
+        }
+
+        return $map;
+    }
 }
diff --git a/src/Service/Yaml/FileToSingleLevelArrayParser.php b/src/Service/Yaml/FileToSingleLevelArrayParser.php
@@ -19,4 +19,17 @@ public function parse(string $path): array
     {
         return iterator_to_array($this->keysLinker->glueKeys($this->fileManipulator->parse($path)));
     }
+
+    /**
+     * @param string[] $files
+     *
+     * @return array<string,string>
+     */
+    public function parseFiles(array $files): array
+    {
+        $yaml = array_merge(...array_map(fn (string $x): array => $this->parse($x), array_values($files)));
+        ksort($yaml);
+
+        return $yaml;
+    }
 }
diff --git a/src/Service/Yaml/FilesFinder.php b/src/Service/Yaml/FilesFinder.php
@@ -25,10 +25,7 @@ public function __construct(DirectoryProvider $directoryProvider, FileMapBuilder
      */
     public function getDomains(): array
     {
-        $domains = array_keys($this->getFilesMap());
-        sort($domains);
-
-        return $domains;
+        return array_keys($this->getFilesMap());
     }
 
     /**

diff --git a/src/Service/Yaml/KeysParser.php b/src/Service/Yaml/KeysParser.php
@@ -21,7 +21,6 @@ public function __construct(FileToSingleLevelArrayParser $fileParser)
     public function getOmittedKeys(array $localesKeys): array
     {
         $allDomainKeys = $this->mergeKeys($localesKeys);
-        ksort($localesKeys);
         $omittedKeys = [];
         foreach ($localesKeys as $locale => $localeKeys) {
             $omittedKeys[$locale] = array_values(array_diff($allDomainKeys, $localeKeys));
@@ -39,7 +38,7 @@ public function getParsedKeys(array $localesFiles): array
     {
         $keys = [];
         foreach ($localesFiles as $locale => $files) {
-            $keys[$locale] = $this->getSortedKeys($this->parseFiles($files));
+            $keys[$locale] = array_keys($this->fileParser->parseFiles($files));
         }
 
         return $keys;
@@ -59,21 +58,14 @@ public function mergeKeys(array $localesKeys): array
     }
 
     /**
+     * @deprecated since version 2.7.0. Use {@see \Aeliot\Bundle\TransMaintain\Service\Yaml\FileToSingleLevelArrayParser::parseFiles() }
+     *
      * @param string[] $files
      *
      * @return array<string,string>
      */
     public function parseFiles(array $files): array
     {
-        // THINK: sort result
-        return array_merge(...array_map(fn (string $x): array => $this->fileParser->parse($x), array_values($files)));
-    }
-
-    private function getSortedKeys(array $values): array
-    {
-        $keys = array_keys($values);
-        sort($keys);
-
-        return $keys;
+        return $this->fileParser->parseFiles($files);
     }
 }
diff --git a/src/Service/Yaml/Linter/EmptyValueLinter.php b/src/Service/Yaml/Linter/EmptyValueLinter.php
@@ -33,22 +33,19 @@ public function lint(LintYamlFilterDto $filterDto): ReportBag
         $domainsFiles = $this->fileMapFilter->getFilesMap($filterDto);
 
         foreach ($domainsFiles as $domain => $localesFiles) {
-            $translationIsWithLocales = $this->getTranslationIdsWithLocalesForEmptyValues($localesFiles);
-            $this->addLines($bag, $domain, $translationIsWithLocales);
+            $translationIdsWithLocales = $this->getTranslationIdsWithLocalesForEmptyValues($localesFiles);
+            $this->addLines($bag, $domain, $translationIdsWithLocales);
         }
 
         return $bag;
     }
 
     /**
-     * @param array<string,array<string>> $translationIsWithLocales
+     * @param array<string,array<string>> $translationIdsWithLocales
      */
-    private function addLines(ReportBag $bag, string $domain, array $translationIsWithLocales): void
+    private function addLines(ReportBag $bag, string $domain, array $translationIdsWithLocales): void
     {
-        ksort($translationIsWithLocales);
-
-        foreach ($translationIsWithLocales as $translationId => $locales) {
-            sort($locales);
+        foreach ($translationIdsWithLocales as $translationId => $locales) {
             $bag->addLine($domain, $translationId, $locales);
         }
     }
@@ -73,20 +70,20 @@ private function createReportBag(): ReportBag
      */
     private function getTranslationIdsWithLocalesForEmptyValues(array $localesFiles): array
     {
-        $translationIsWithLocales = [];
+        $translationIdsWithLocales = [];
         foreach ($localesFiles as $locale => $files) {
-            foreach ($files as $file) {
-                foreach ($this->fileParser->parse($file) as $translationId => $value) {
-                    if ('' === trim($value)) {
-                        if (!\array_key_exists($translationId, $translationIsWithLocales)) {
-                            $translationIsWithLocales[$translationId] = [];
-                        }
-                        $translationIsWithLocales[$translationId][] = $locale;
+            foreach ($this->fileParser->parseFiles($files) as $translationId => $value) {
+                if ('' === trim($value)) {
+                    if (!\array_key_exists($translationId, $translationIdsWithLocales)) {
+                        $translationIdsWithLocales[$translationId] = [];
                     }
+                    $translationIdsWithLocales[$translationId][] = $locale;
                 }
             }
         }
 
-        return $translationIsWithLocales;
+        ksort($translationIdsWithLocales);
+
+        return $translationIdsWithLocales;
     }
 }
diff --git a/src/Service/Yaml/Linter/InvalidValueLinter.php b/src/Service/Yaml/Linter/InvalidValueLinter.php
@@ -85,11 +85,9 @@ private function getInvalidValueKeys(array $files): array
     {
         $invalidValueKeys = [];
 
-        foreach ($files as $file) {
-            foreach ($this->fileParser->parse($file) as $translationId => $value) {
-                if (preg_match($this->invalidValuePattern, $value)) {
-                    $invalidValueKeys[] = $translationId;
-                }
+        foreach ($this->fileParser->parseFiles($files) as $translationId => $value) {
+            if (preg_match($this->invalidValuePattern, $value)) {
+                $invalidValueKeys[] = $translationId;
             }
         }
 

diff --git a/src/Service/Yaml/Linter/KeysDuplicatedLinter.php b/src/Service/Yaml/Linter/KeysDuplicatedLinter.php
@@ -50,9 +50,6 @@ public function lint(LintYamlFilterDto $filterDto): ReportBag
      */
     private function addLines(ReportBag $bag, string $domain, string $locale, array $duplicatedKeys): void
     {
-        $duplicatedKeys = array_unique($duplicatedKeys);
-        sort($duplicatedKeys);
-
         foreach ($duplicatedKeys as $translationId) {
             $bag->addLine($domain, $locale, $translationId);
         }
@@ -89,6 +86,9 @@ private function getDuplicatedKeys(array $files): array
                 }
             }
         }
+        unset($values);
+        $duplicatedKeys = array_unique($duplicatedKeys);
+        sort($duplicatedKeys);
 
         return $duplicatedKeys;
     }

diff --git a/src/Service/Yaml/Linter/KeysMissedLinter.php b/src/Service/Yaml/Linter/KeysMissedLinter.php
@@ -33,6 +33,7 @@ public function getPresets(): array
     public function lint(LintYamlFilterDto $filterDto): ReportBag
     {
         $bag = $this->createReportBag();
+        // NOTE: don't use filtered files map! It may lead to incorrect interpreting single locale of domain.
         $domainsFiles = $this->filesFinder->getFilesMap();
         foreach ($domainsFiles as $domain => $localesFiles) {
             if ($filterDto->domains && !\in_array($domain, $filterDto->domains, true)) {
@@ -50,7 +51,6 @@ public function lint(LintYamlFilterDto $filterDto): ReportBag
                 $omittedLocales = $this->getOmittedLocales($omittedKeys, (string) $translationId, $filterDto->locales);
 
                 if ($omittedLocales) {
-                    sort($omittedLocales);
                     $bag->addLine($domain, $translationId, $omittedLocales);
                 }
             }

diff --git a/src/Service/Yaml/Linter/SameValueLinter.php b/src/Service/Yaml/Linter/SameValueLinter.php
@@ -47,10 +47,7 @@ public function lint(LintYamlFilterDto $filterDto): ReportBag
      */
     private function addLines(ReportBag $bag, string $domain, string $locale, array $same): void
     {
-        ksort($same);
-
         foreach ($same as $translation => $translationIds) {
-            sort($translationIds);
             $bag->addLine($domain, $locale, $translation, $translationIds);
         }
     }
@@ -78,19 +75,19 @@ private function getTranslationKeysWithSameValues(array $files): array
     {
         $same = [];
         $values = [];
-        foreach ($files as $file) {
-            foreach ($this->fileParser->parse($file) as $translationId => $translation) {
-                if (!\array_key_exists($translation, $values)) {
-                    $values[$translation] = $translationId;
-                } else {
-                    if (!\array_key_exists($translation, $same)) {
-                        $same[$translation] = [$values[$translation]];
-                    }
-
-                    $same[$translation][] = $translationId;
+        foreach ($this->fileParser->parseFiles($files) as $translationId => $translation) {
+            if (!\array_key_exists($translation, $values)) {
+                $values[$translation] = $translationId;
+            } else {
+                if (!\array_key_exists($translation, $same)) {
+                    $same[$translation] = [$values[$translation]];
                 }
+
+                $same[$translation][] = $translationId;
             }
         }
+        unset($values);
+        ksort($same);
 
         return $same;
     }

diff --git a/src/Service/Yaml/MissedValuesFinder.php b/src/Service/Yaml/MissedValuesFinder.php
@@ -7,11 +7,13 @@
 final class MissedValuesFinder
 {
     private FilesFinder $filesFinder;
+    private FileToSingleLevelArrayParser $fileParser;
     private KeysParser $keysParser;
 
-    public function __construct(FilesFinder $filesFinder, KeysParser $keysParser)
+    public function __construct(FilesFinder $filesFinder, FileToSingleLevelArrayParser $fileParser, KeysParser $keysParser)
     {
         $this->filesFinder = $filesFinder;
+        $this->fileParser = $fileParser;
         $this->keysParser = $keysParser;
     }
 
@@ -28,7 +30,7 @@ public function findMissedTranslations(string $domain, string $sourceLocale, ?st
         $allOmittedKeys = $this->keysParser->mergeKeys($omittedKeys);
 
         $values = array_intersect_key(
-            $this->keysParser->parseFiles($domainsFiles[$domain][$sourceLocale]),
+            $this->fileParser->parseFiles($domainsFiles[$domain][$sourceLocale]),
             array_flip($allOmittedKeys)
         );
 
@@ -40,8 +42,6 @@ public function findMissedTranslations(string $domain, string $sourceLocale, ?st
             $values = array_intersect_key($values, array_flip($filterKeys));
         }
 
-        ksort($values);
-
         return $values;
     }
 }
diff --git a/tests/Unit/Model/ReportBagTest.php b/tests/Unit/Model/ReportBagTest.php
@@ -59,7 +59,7 @@ public function testInvalidValueType(array $columnConfig, array $lineValues): vo
 
     public function testLineColumnNames(): void
     {
-        //TODO implement testing if report line columns is same to configured
+        // TODO implement testing if report line columns is same to configured
         $this->markTestSkipped('Test not implemented yet');
     }
 

diff --git a/tests/Unit/Service/LocalesDetectorTest.php b/tests/Unit/Service/LocalesDetectorTest.php
@@ -1,5 +1,7 @@
 <?php
 
+declare(strict_types=1);
+
 namespace Aeliot\Bundle\TransMaintain\Test\Unit\Service;
 
 use Aeliot\Bundle\TransMaintain\Service\LocalesDetector;
@@ -47,4 +49,4 @@ private function createLocalesDetector(array $filesMap): LocalesDetector
     {
         return new LocalesDetector($this->mockFilesFinder($filesMap, $this));
     }
-}
+}
diff --git a/tests/Unit/Service/Yaml/FileMapFilterTest.php b/tests/Unit/Service/Yaml/FileMapFilterTest.php
@@ -71,10 +71,6 @@ public function getDataForFiltering(): \Generator
         ];
     }
 
-    /**
-     * @param array $filesMap
-     * @return FileMapFilter
-     */
     private function createFileMapFilter(array $filesMap): FileMapFilter
     {
         return new FileMapFilter($this->mockFilesFinder($filesMap, $this));

diff --git a/tests/Unit/Service/Yaml/FileToSingleLevelArrayParserTest.php b/tests/Unit/Service/Yaml/FileToSingleLevelArrayParserTest.php
@@ -20,7 +20,21 @@ final class FileToSingleLevelArrayParserTest extends TestCase
      */
     public function testParse(array $expected, array $value): void
     {
-        self::assertSame($expected, $this->createParser($value)->parse('some_path'));
+        $parser = new FileToSingleLevelArrayParser($this->mockFileManipulatorSingle($value, $this), new KeysLinker());
+        self::assertSame($expected, $parser->parse('some_path'));
+    }
+
+    /**
+     * @dataProvider getDataForTestParseFiles
+     *
+     * @param array<string,string> $expected
+     * @param string[] $files
+     * @param array<string,mixed> $fileTranslations
+     */
+    public function testParseFiles(array $expected, array $files, array $fileTranslations): void
+    {
+        $parser = new FileToSingleLevelArrayParser($this->mockFileManipulatorMultiple($fileTranslations, $this), new KeysLinker());
+        self::assertSame($expected, $parser->parseFiles($files));
     }
 
     public function getDataForTestParse(): \Generator
@@ -31,11 +45,15 @@ public function getDataForTestParse(): \Generator
         ];
     }
 
-    /**
-     * @param array<string,array<string,array<int,string>>> $value
-     */
-    private function createParser(array $value): FileToSingleLevelArrayParser
+    public function getDataForTestParseFiles(): \Generator
     {
-        return new FileToSingleLevelArrayParser($this->mockFileManipulatorSingle($value, $this), new KeysLinker());
+        yield [
+            ['a' => '*', 'b' => '*', 'c' => '*'],
+            ['/var/a/message.en.yaml', '/var/b/message.en.yaml'],
+            [
+                '/var/a/message.en.yaml' => ['a' => '*', 'c' => '*'],
+                '/var/b/message.en.yaml' => ['b' => '*'],
+            ],
+        ];
     }
 }