launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/henninge_validate_translation-cleanup into lp:~launchpad/launchpad/recife.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers): code


= Henning's validate_translation cleanup =

For the Recife feature branch.  I extracted this from Henning's ongoing feature work in his absence, since it'll take a lot of diff out of his main branch.

What it does is make the validate_translation helper conform to our agreed standard way of passing translation strings around: as a dict mapping plural-form numbers to translation strings.  We used a mixture of these dicts and None-augmented lists in the past, and that is also what validate_translation accepted.

En passant this also eliminates another helper.  Unlike msgstrs, msgids have a fixed set of plural forms: "normal" messages have only a singular and an optional plural.  There's no need to generalize that into a list.

To test this, best run all the Translations tests:
{{{
./bin/test -vvc lp.translations
}}}

No lint.


Jeroen
-- 
https://code.launchpad.net/~jtv/launchpad/henninge_validate_translation-cleanup/+merge/36683
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/henninge_validate_translation-cleanup into lp:~launchpad/launchpad/recife.

=== modified file 'lib/lp/translations/model/potmsgset.py'
--- lib/lp/translations/model/potmsgset.py	2010-09-08 02:40:38 +0000
+++ lib/lp/translations/model/potmsgset.py	2010-09-27 08:14:43 +0000
@@ -489,15 +489,6 @@
             date_updated = current.date_reviewed
         return (date_updated is not None and date_updated > timestamp)
 
-    def _list_of_msgids(self):
-        """Return a list of [singular_text, plural_text] if the message
-        is using plural forms, or just [singular_text] if it's not.
-        """
-        original_texts = [self.singular_text]
-        if self.plural_text is not None:
-            original_texts.append(self.plural_text)
-        return original_texts
-
     def _sanitizeTranslations(self, translations, pluralforms):
         """Sanitize `translations` using self.applySanityFixes.
 
@@ -528,14 +519,12 @@
         # By default all translations are correct.
         validation_status = TranslationValidationStatus.OK
 
-        # Cache the list of singular_text and plural_text
-        original_texts = self._list_of_msgids()
-
         # Validate the translation we got from the translation form
         # to know if gettext is unhappy with the input.
         try:
             validate_translation(
-                original_texts, translations, self.flags)
+                self.singular_text, self.plural_text,
+                translations, self.flags)
         except GettextValidationError:
             if ignore_errors:
                 # The translations are stored anyway, but we set them as
@@ -543,13 +532,7 @@
                 validation_status = TranslationValidationStatus.UNKNOWNERROR
             else:
                 # Check to know if there is any translation.
-                has_translations = False
-                for key in translations.keys():
-                    if translations[key] is not None:
-                        has_translations = True
-                        break
-
-                if has_translations:
+                if any(translations.values()):
                     # Partial translations cannot be stored, the
                     # exception is raised again and handled outside
                     # this method.

=== modified file 'lib/lp/translations/scripts/gettext_check_messages.py'
--- lib/lp/translations/scripts/gettext_check_messages.py	2010-09-06 10:40:54 +0000
+++ lib/lp/translations/scripts/gettext_check_messages.py	2010-09-27 08:14:43 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -10,7 +10,6 @@
     timedelta,
     )
 
-from storm.locals import Store
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
@@ -95,11 +94,13 @@
         :return: Error message string if there is an error, or None otherwise.
         """
         potmsgset = translationmessage.potmsgset
-        msgids = potmsgset._list_of_msgids()
-        msgstrs = translationmessage.translations
+        # validate_translation takes a dict but translations are a list.
+        msgstrs = dict(enumerate(translationmessage.translations))
 
         try:
-            validate_translation(msgids, msgstrs, potmsgset.flags)
+            validate_translation(
+                potmsgset.singular_text, potmsgset.plural_text,
+                msgstrs, potmsgset.flags)
         except GettextValidationError, error:
             self._error_count += 1
             return unicode(error)

=== modified file 'lib/lp/translations/utilities/tests/test_validate.py'
--- lib/lp/translations/utilities/tests/test_validate.py	2010-08-23 08:41:03 +0000
+++ lib/lp/translations/utilities/tests/test_validate.py	2010-09-27 08:14:43 +0000
@@ -17,46 +17,60 @@
 
     def test_validate_translation_c_format(self):
         # Correct c-format translations will be validated.
-        english = ["English %s number %d"]
+        english = "English %s number %d"
         flags = ["c-format"]
         translations = {0: "Translation %s number %d"}
         # This should not raise GettextValidationError.
-        validate_translation(english, translations, flags)
+        validate_translation(english, None, translations, flags)
 
     def test_validate_translation_c_format_fail(self):
         # Mismatched format specifiers will not be validated.
-        english = ["English %s number %d"]
+        english = "English %s number %d"
         flags = ["c-format"]
         translations = {0: "Translation %d"}
         self.assertRaises(
             GettextValidationError,
-            validate_translation, english, translations, flags)
+            validate_translation, english, None, translations, flags)
+
+    def test_validate_translation_no_flag(self):
+        # Mismatched format specifiers don't matter if no format has been
+        # specified.
+        english = "English %s number %d"
+        flags = []
+        translations = {0: "Translation number %d"}
+        # This should not raise GettextValidationError.
+        validate_translation(english, None, translations, flags)
 
     def test_validate_translation_c_format_plural(self):
         # Correct c-format translations will be validated on plurals.
-        english = ["English %s number %d", "English plural %s number %d"]
+        english_singular = "English %s number %d"
+        english_plural = "English plural %s number %d"
         flags = ["c-format"]
         translations = {
             0: "Translation singular %s number %d",
             1: "Translation plural %s number %d",
             }
         # This should not raise GettextValidationError.
-        validate_translation(english, translations, flags)
+        validate_translation(
+            english_singular, english_plural, translations, flags)
 
     def test_validate_translation_c_format_plural_no_singular_format(self):
         # As a special case, the singular does not need format specifiers.
-        english = ["English %s number %d", "English plural %s number %d"]
+        english_singular = "English %s number %d"
+        english_plural = "English plural %s number %d"
         flags = ["c-format"]
         translations = {
             0: "Translation singular",
             1: "Translation plural %s number %d",
             }
         # This should not raise GettextValidationError.
-        validate_translation(english, translations, flags)
+        validate_translation(
+            english_singular, english_plural, translations, flags)
 
     def test_validate_translation_c_format_plural_fail(self):
         # Not matching format specifiers will not be validated.
-        english = ["English %s number %d", "English plural %s number %d"]
+        english_singular = "English %s number %d"
+        english_plural = "English plural %s number %d"
         flags = ["c-format"]
         translations = {
             0: "Translation singular %d",
@@ -64,7 +78,8 @@
             }
         self.assertRaises(
             GettextValidationError,
-            validate_translation, english, translations, flags)
+            validate_translation, english_singular, english_plural,
+            translations, flags)
 
 
 def test_suite():

=== modified file 'lib/lp/translations/utilities/validate.py'
--- lib/lp/translations/utilities/validate.py	2010-03-26 19:37:58 +0000
+++ lib/lp/translations/utilities/validate.py	2010-09-27 08:14:43 +0000
@@ -15,27 +15,29 @@
     """Gettext validation failed."""
 
 
-def validate_translation(original, translation, flags):
+def validate_translation(original_singular, original_plural,
+                         translations, flags):
     """Check with gettext if a translation is correct or not.
 
     If the translation has a problem, raise `GettextValidationError`.
 
-    :param msgids: A list of one or two msgids, depending on whether the
-        message has a plural.
-    :param translation: A dictionary of translations, indexed with the plural
+    :param original_singular: The English msgid.
+    :param original_plural: The English plural msgid, if the message has a
+        plural or None otherwise.
+    :param translations: A dictionary of translations, indexed with the plural
         form number.
     :param flags: This message's flags as a list of strings.
     """
     msg = gettextpo.PoMessage()
-    msg.set_msgid(original[0])
+    msg.set_msgid(original_singular)
 
-    if len(original) > 1:
+    if original_plural is not None:
         # It has plural forms.
-        msg.set_msgid_plural(original[1])
-        for form in range(len(translation)):
-            msg.set_msgstr_plural(form, translation[form])
-    elif len(translation) > 0:
-        msg.set_msgstr(translation[0])
+        msg.set_msgid_plural(original_plural)
+        for form, translation in translations.iteritems():
+            msg.set_msgstr_plural(form, translation)
+    elif 0 in translations:
+        msg.set_msgstr(translations[0])
     else:
         pass
 
@@ -48,4 +50,3 @@
     except gettextpo.error, e:
         # Wrap gettextpo.error in GettextValidationError.
         raise GettextValidationError(unicode(e))
-