Switch to using msgfmt to check for format errors in translated strings

It catches more than pofilter with fewer false positives.

setup/__init__.py

@@ -166,6 +166,12 @@ def get_warnings():
     return list(warnings)
 
 
+def edit_file(path):
+    return subprocess.Popen([
+        'vim', '-c', 'SyntasticCheck', '-c', 'll', '-S', os.path.join(SRC, '../session.vim'), '-f', path
+    ]).wait() == 0
+
+
 class Command(object):
 
     SRC = SRC

setup/check.py

@@ -7,7 +7,7 @@ __copyright__ = '2009, Kovid Goyal <kovid@kovidgoyal.net>'
 __docformat__ = 'restructuredtext en'
 
 import sys, os, json, subprocess, errno, hashlib
-from setup import Command, build_cache_dir
+from setup import Command, build_cache_dir, edit_file
 import __builtin__
 
 
@@ -119,8 +119,7 @@ class Check(Command):
                 self.info('\tChecking', f)
                 if self.file_has_errors(f):
                     self.info('%d files left to check' % (len(dirty_files) - i - 1))
-                    subprocess.call(['vim', '-c', 'SyntasticCheck', '-c', 'll', '-S',
+                    edit_file(f)
-                                    self.j(self.SRC, '../session.vim'), '-f', f])
                     if self.file_has_errors(f):
                         raise SystemExit(1)
                 cache[f] = self.file_hash(f)

setup/translations.py

@@ -11,7 +11,7 @@ from collections import defaultdict
 from locale import normalize as normalize_locale
 from functools import partial
 
-from setup import Command, __appname__, __version__, require_git_master, build_cache_dir
+from setup import Command, __appname__, __version__, require_git_master, build_cache_dir, edit_file
 from setup.parallel_build import parallel_check_output
 is_ci = os.environ.get('CI', '').lower() == 'true'
 
@@ -511,8 +511,14 @@ class GetTranslations(Translations):  # {{{
     def is_modified(self):
         return bool(subprocess.check_output('git status --porcelain'.split(), cwd=self.TRANSLATIONS))
 
+    def add_options(self, parser):
+        parser.add_option('-e', '--check-for-errors', default=False, action='store_true',
+                          help='Check for errors in .po files')
+
     def run(self, opts):
         require_git_master()
+        if opts.check_for_errors:
+            return self.check_for_errors()
         self.tx('pull -a')
         if self.is_modified:
             self.check_for_errors()
@@ -551,23 +557,54 @@ class GetTranslations(Translations):  # {{{
             self.tx('push -r calibre.%s -t -l %s' % (slug, ','.join(languages)))
 
     def check_for_errors(self):
+        self.info('Checking for errors in .po files...')
+        groups = 'calibre content-server website'.split()
+        for group in groups:
+            self.check_group(group)
+        self.check_website()
+        for group in groups:
+            self.push_fixes(group)
+
+    def push_fixes(self, group):
+        languages = set()
+        for line in subprocess.check_output('git status --porcelain'.split(), cwd=self.TRANSLATIONS).decode('utf-8').splitlines():
+            parts = line.strip().split()
+            if len(parts) > 1 and 'M' in parts[0] and parts[-1].startswith(group + '/') and parts[-1].endswith('.po'):
+                languages.add(os.path.basename(parts[-1]).partition('.')[0])
+        if languages:
+            pot = 'main' if group == 'calibre' else group.replace('-', '_')
+            print('Pushing fixes for %s.pot languages: %s' % (pot, ', '.join(languages)))
+            self.tx('push -r calibre.{} -t -l '.format(pot) + ','.join(languages))
+
+    def check_group(self, group):
+        files = glob.glob(os.path.join(self.TRANSLATIONS, group, '*.po'))
+        cmd = ['msgfmt', '-o', os.devnull, '--check-format']
+        # Disabled because too many such errors, and not that critical anyway
+        # if group == 'calibre':
+        #     cmd += ['--check-accelerators=&']
+
+        def check(f):
+            p = subprocess.Popen(cmd + [f], stderr=subprocess.PIPE)
+            errs = p.stderr.read()
+            p.wait()
+            return errs
+
+        for f in files:
+            errs = check(f)
+            if errs:
+                print(f)
+                print(errs)
+                edit_file(f)
+                if check(f):
+                    raise SystemExit('Aborting as not all errors were fixed')
+
+    def check_website(self):
         errors = os.path.join(tempfile.gettempdir(), 'calibre-translation-errors')
         if os.path.exists(errors):
             shutil.rmtree(errors)
         os.mkdir(errors)
-        tpath = self.j(self.TRANSLATIONS, __appname__)
+        tpath = self.j(self.TRANSLATIONS, 'website')
-        pofilter = ('pofilter', '-i', tpath, '-o', errors,
+        pofilter = ('pofilter', '-i', tpath, '-o', errors, '-t', 'xmltags')
-                '-t', 'accelerators', '-t', 'escapes', '-t', 'variables',
-                '-t', 'pythonbraceformat',
-                # '-t', 'xmltags',
-                # '-t', 'brackets',
-                # '-t', 'emails',
-                # '-t', 'doublequoting',
-                # '-t', 'filepaths',
-                # '-t', 'numbers',
-                '-t', 'options',
-                # '-t', 'urls',
-                '-t', 'printf')
         subprocess.check_call(pofilter)
         errfiles = glob.glob(errors+os.sep+'*.po')
         if errfiles:
@@ -581,15 +618,6 @@ class GetTranslations(Translations):  # {{{
                     f.write(raw)
 
             subprocess.check_call(['pomerge', '-t', tpath, '-i', errors, '-o', tpath])
-            languages = []
-            for f in glob.glob(self.j(errors, '*.po')):
-                lc = os.path.basename(f).rpartition('.')[0]
-                languages.append(lc)
-            if languages:
-                print('Pushing fixes for languages: %s' % (', '.join(languages)))
-                self.tx('push -r calibre.main -t -l ' + ','.join(languages))
-            return True
-        return False
 
     def upload_to_vcs(self):
         print ('Uploading updated translations to version control')