Merge pull request #95 from overhangio/overhang/fix-missing-sources

fix: do not skip po file generation on missing sources

i18n/generate.py

@@ -43,15 +43,19 @@ def merge(configuration, locale, target='django.po', sources=('django-partial.po
     """
     LOG.info('Merging %s locale %s', target, locale)
     locale_directory = configuration.get_messages_dir(locale)
-    try:
-        validate_files(locale_directory, sources)
-    except Exception:
-        if not fail_if_missing:
-            return
-        raise
+    valid_sources = []
+    for filename in sources:
+        try:
+            validate_file(locale_directory, filename)
+            valid_sources.append(filename)
+        except (ValueError, IOError):
+            if fail_if_missing:
+                raise
+    if not valid_sources:
+        return
 
     # merged file is merged.po
-    merge_cmd = 'msgcat -o merged.po ' + ' '.join(sources)
+    merge_cmd = 'msgcat -o merged.po ' + ' '.join(valid_sources)
     execute(merge_cmd, working_directory=locale_directory)
 
     # clean up redunancies in the metadata
@@ -135,23 +139,23 @@ def clean_pofile(pofile_path):
     return duplicate_entries
 
 
-def validate_files(directory, files_to_merge):
+def validate_file(directory, filename):
     """
     Asserts that the given files exist.
     files_to_merge is a list of file names (no directories).
     directory is the directory (a path object from path.py) in which the files should appear.
     raises an Exception if any of the files are not in dir.
     """
-    for file_path in files_to_merge:
-        pathname = directory.joinpath(file_path)
-        if not pathname.exists():
-            raise Exception(f"I18N: Cannot generate because file not found: {pathname}")
-        # clean sources
-        clean_pofile(pathname)
+    pathname = directory.joinpath(filename)
+    if not pathname.exists():
+        raise ValueError(f"I18N: Cannot generate because file not found: {pathname}")
+    # clean sources
+    clean_pofile(pathname)
 
 
 class Generate(Runner):
     """Generate merged and compiled message files."""
+
     def add_args(self):
         self.parser.description = "Generate merged and compiled message files."
         self.parser.add_argument("--strict", action='store_true', help="Complain about missing files.")

setup.py

@@ -27,7 +27,7 @@ def is_requirement(line):
 
 setup(
     name='edx-i18n-tools',
-    version='0.5.3',
+    version='0.6.0',
     description='edX Internationalization Tools',
     author='edX',
     author_email='oscm@edx.org',

tests/data/mock-django-app/locale/mock/LC_MESSAGES/django-partial.po

@@ -11,10 +11,10 @@ msgstr ""
 "PO-Revision-Date: 2015-06-15 20:15:26.289785\n"
 "Last-Translator: \n"
 "Language-Team: openedx-translation <openedx-translation@googlegroups.com>\n"
+"Language: en\n"
 "MIME-Version: 1.0\n"
 "Content-Type: text/plain; charset=UTF-8\n"
 "Content-Transfer-Encoding: 8bit\n"
-"Language: en\n"
 "Plural-Forms: nplurals=2; plural=(n != 1);\n"
 
 #. Translators: 'Discussion' refers to the tab in the courseware that leads to

tests/test_generate.py

@@ -2,6 +2,7 @@
 This test tests that i18n extraction works properly.
 """
 from datetime import datetime, timedelta
+from filecmp import dircmp
 import random
 import re
 import string
@@ -20,6 +21,7 @@ class TestGenerate(I18nToolTestCase):
     """
     Tests functionality of i18n/generate.py
     """
+
     def setUp(self):
         self.mock_path = Path.joinpath(MOCK_APPLICATION_DIR, "conf", "locale", "mock")
         self.fr_path = Path.joinpath(MOCK_APPLICATION_DIR, "conf", "locale", "fr")
@@ -45,6 +47,38 @@ def test_merge(self):
         self.assertTrue(Path.exists(filename))
         Path.remove(filename)
 
+    def test_merge_with_missing_sources(self):
+        """
+        Tests merge script when some source files are missing. The existing files should be merged anyway.
+        """
+        test_configuration = config.Configuration(root_dir=MOCK_DJANGO_APP_DIR)
+        filename = Path.joinpath(test_configuration.source_messages_dir, random_name())
+        generate.merge(
+            test_configuration,
+            test_configuration.source_locale,
+            sources=("django-partial.po", "nonexistingfile.po"),
+            target=filename,
+            fail_if_missing=False,
+        )
+        self.assertTrue(Path.exists(filename))
+        Path.remove(filename)
+
+    def test_merge_with_missing_sources_strict(self):
+        """
+        Tests merge script when some source files are missing. In strict mode, an exception should be raised.
+        """
+        test_configuration = config.Configuration(root_dir=MOCK_DJANGO_APP_DIR)
+        filename = Path.joinpath(test_configuration.source_messages_dir, random_name())
+        with self.assertRaises(ValueError):
+            generate.merge(
+                test_configuration,
+                test_configuration.source_locale,
+                sources=("django-partial.po", "nonexistingfile.po"),
+                target=filename,
+                fail_if_missing=True,
+            )
+        self.assertFalse(Path.exists(filename))
+
     # Patch dummy_locales to not have esperanto present
     def test_main(self):
         """
@@ -61,8 +95,9 @@ def test_main(self):
                 file_path = Path.joinpath(self.configuration.get_messages_dir(locale), mofile)
                 exists = Path.exists(file_path)
                 self.assertTrue(exists, msg='Missing file in locale %s: %s' % (locale, mofile))
-                self.assertTrue(
-                    datetime.fromtimestamp(Path.getmtime(file_path), UTC) >= self.start_time,
+                self.assertGreaterEqual(
+                    datetime.fromtimestamp(Path.getmtime(file_path), UTC),
+                    self.start_time,
                     msg='File should be recently modified: %s' % file_path
                 )
 
@@ -115,14 +150,13 @@ def test_resolve_merge_conflicts(self, mock_log):
 
     def test_lang_mapping(self):
         generate.main(verbose=0, strict=False, root_dir=MOCK_APPLICATION_DIR)
-        self.assertTrue(len(self.configuration.edx_lang_map) > 0)
+        self.assertGreater(len(self.configuration.edx_lang_map), 0)
 
         for source_locale, dest_locale in self.configuration.edx_lang_map.items():
             source_dirname = self.configuration.get_messages_dir(source_locale)
             dest_dirname = self.configuration.get_messages_dir(dest_locale)
 
             self.assertTrue(Path.exists(dest_dirname))
-            from filecmp import dircmp
 
             diff = dircmp(source_dirname, dest_dirname)
             self.assertEqual(len(diff.left_only), 0)