Merge lp:~henninge/launchpad/bug-456498 into lp:launchpad

[Henning Eggers]

[Henning Eggers (henninge)]

= Details =

See bug 456498.
This is a simiple refactoring so that the test for strings (msgids) that identify translator credits are not checked in two different places in the code. This makes the code clearer and future additions of translator credit strings would be easier (if they ever happen).

== Implementation notes ==

The property translation_credit_type is a string that is a normalized version of the possible msgids for this type. It can be considred to be an enum value but this implementation is simpler and more fitting for this quick refactoring.

== Test ==

bin/test -vvct potmsgset.txt

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/translations/doc/potmsgset.txt
  lib/lp/translations/interfaces/potmsgset.py
  lib/lp/translations/model/pofile.py
  lib/lp/translations/model/potmsgset.py

[Brad Crittenden (bac)]

Hi Henning,

* Please whack line 5 and just use pytz.UTC throughout.

* You agreed to reword the sentence at line 12 to read better.

* We also discussed biting the bullet and making the trans_credit_type an enum since having the magic strings scattered around make me a little queasy. I think/hope you'll find it isn't much worse.

I'm marking this Needs Fixing and ask that you paste an incremental diff so we can determine if it was worth the effort.

Thanks for the branch Henning.

review: Needs Fixing (code)

[Henning Eggers (henninge)]

Hi Brad,
here is the patch with the changes. It has gotten lengthier because or the pytz.UTC change and because I ordered some imports properly.
Thank you for the review and prodding me to do this properly using and enum. Danilo is thrilled! ;-)

Henning

[Henning Eggers (henninge)]

=== modified file 'lib/lp/translations/doc/potmsgset.txt'
--- lib/lp/translations/doc/potmsgset.txt 2009-10-20 18:06:19 +0000
+++ lib/lp/translations/doc/potmsgset.txt 2009-10-21 10:07:54 +0000
@@ -25,43 +25,43 @@

     >>> from datetime import datetime
     >>> import pytz
- >>> UTC = pytz.timezone('UTC')

 == POTMsgSet.is_translation_credit and POTMsgSet.translation_credit_type ==

-A POTMsgSet can be a translation credit. These have special msgids that may
-differ for historical reason for the same type of credit. Two methods detect
-gloss over these different msgids.
+A POTMsgSet can be translation credits. These have special msgids that may
+differ for historical reason for the same type of credit. The property
+is_translation_credit indicates if the POTMsgSet is translation credits. The
+property translation_credit_type contains the type of translation credits.

     >>> def print_credit(credit):
     ... print credit.is_translation_credit
- ... print credit.translation_credit_type
+ ... print credit.translation_credits_type.title

     >>> print_credit(factory.makePOTMsgSet(potemplate))
     False
- None
+ Not a translation credits message

     >>> print_credit(factory.makePOTMsgSet(
     ... potemplate, singular=u'translator-credits'))
     True
- translator-credits
+ Gnome credits message

     >>> print_credit(factory.makePOTMsgSet(
     ... potemplate, singular=u'translation-credits'))
     True
- translator-credits
+ Gnome credits message

     >>> print_credit(factory.makePOTMsgSet(
     ... potemplate,
     ... singular=u'Your emails', context=u'EMAIL OF TRANSLATORS'))
     True
- Your emails
+ KDE emails credits message

     >>> print_credit(factory.makePOTMsgSet(
     ... potemplate, singular=u'_: EMAIL OF TRANSLATORS\nYour emails'))
     True
- Your emails
+ KDE emails credits message

 == POTMsgSet.normalizeWhitespaces ==
@@ -256,7 +256,7 @@

     >>> new_translation = upstream_potmsgset.updateTranslation(upstream_pofile,
     ... carlos, {0: u'foo'}, is_imported=False, force_suggestion=True,
- ... lock_timestamp=datetime.now(UTC))
+ ... lock_timestamp=datetime.now(pytz.UTC))

 Since this message is just a suggestion, it doesn't have a reviewer, but
 it does note a person which last changed it.
@@ -301,7 +301,7 @@

     >>> new_translation = upstream_potmsgset.updateTranslation(
     ... upstream_pofile, carlos, {0: u'foo %d', 1: u'foos %d'},
- ... is_imported=True, lock_timestamp=datetime.now(UTC))
+ ... is_imported=True, lock_timestamp=datetime.now(pytz.UTC))

 The new translation is reflected in the POFile statistics.

@@ -330,7 +330,7 @@
     ... u'have ')
     >>> new_translation = upstream_potmsgset.updateTranslation(
     ... upstream_pofile, carlos, {0: u''},
- ... is_imported=True, lock_timestamp=datetime.now(UTC))
+ ... is_imported=True, lock_timestamp=datetime.now(pytz.UTC))

 Current translation is now unset, as is imported.

@@ -354,7 +354,7 @@
     ... u'%d contact', u'%d contacts')
     >>> new_translation = upstream_potmsgset.updateTranslation(
     ... ...

[Brad Crittenden (bac)]

Henning this incremental diff looks fantastic! Thanks for making the change.

Not to be too immodest, but this conversation we just had is what peer code reviews should be: discourse-driven, collaborative, respectful and successful in that the resulting code is improved. Thanks for making it happen.

review: Approve (code)

=== modified file 'lib/lp/translations/doc/potmsgset.txt'
--- lib/lp/translations/doc/potmsgset.txt	2009-07-02 17:16:50 +0000
+++ lib/lp/translations/doc/potmsgset.txt	2009-10-21 10:17:14 +0000
@@ -25,7 +25,43 @@
 
     >>> from datetime import datetime
     >>> import pytz
-    >>> UTC = pytz.timezone('UTC')
+
+
+== POTMsgSet.is_translation_credit and POTMsgSet.translation_credit_type ==
+
+A POTMsgSet can be translation credits. These have special msgids that may
+differ for historical reason for the same type of credit. The property
+is_translation_credit indicates if the POTMsgSet is translation credits. The
+property translation_credit_type contains the type of translation credits.
+
+    >>> def print_credit(credit):
+    ...     print credit.is_translation_credit
+    ...     print credit.translation_credits_type.title
+
+    >>> print_credit(factory.makePOTMsgSet(potemplate))
+    False
+    Not a translation credits message
+
+    >>> print_credit(factory.makePOTMsgSet(
+    ...     potemplate, singular=u'translator-credits'))
+    True
+    Gnome credits message
+
+    >>> print_credit(factory.makePOTMsgSet(
+    ...     potemplate, singular=u'translation-credits'))
+    True
+    Gnome credits message
+
+    >>> print_credit(factory.makePOTMsgSet(
+    ...     potemplate,
+    ...     singular=u'Your emails', context=u'EMAIL OF TRANSLATORS'))
+    True
+    KDE emails credits message
+
+    >>> print_credit(factory.makePOTMsgSet(
+    ...     potemplate, singular=u'_: EMAIL OF TRANSLATORS\nYour emails'))
+    True
+    KDE emails credits message
 
 
 == POTMsgSet.normalizeWhitespaces ==
@@ -220,7 +256,7 @@
 
     >>> new_translation = upstream_potmsgset.updateTranslation(upstream_pofile,
     ...     carlos, {0: u'foo'}, is_imported=False, force_suggestion=True,
-    ...     lock_timestamp=datetime.now(UTC))
+    ...     lock_timestamp=datetime.now(pytz.UTC))
 
 Since this message is just a suggestion, it doesn't have a reviewer, but
 it does note a person which last changed it.
@@ -265,7 +301,7 @@
 
     >>> new_translation = upstream_potmsgset.updateTranslation(
     ...     upstream_pofile, carlos, {0: u'foo %d', 1: u'foos %d'},
-    ...     is_imported=True, lock_timestamp=datetime.now(UTC))
+    ...     is_imported=True, lock_timestamp=datetime.now(pytz.UTC))
 
 The new translation is reflected in the POFile statistics.
 
@@ -294,7 +330,7 @@
     ...     u'have ')
     >>> new_translation = upstream_potmsgset.updateTranslation(
     ...     upstream_pofile, carlos, {0: u''},
-    ...     is_imported=True, lock_timestamp=datetime.now(UTC))
+    ...     is_imported=True, lock_timestamp=datetime.now(pytz.UTC))
 
 Current translation is now unset, as is imported.
 
@@ -318,7 +354,7 @@
     ...     u'%d contact', u'%d contacts')
     >>> new_translation = upstream_potmsgset.updateTranslation(
     ...     upstream_pofile, carlos, {0: u'', 1: u''},
-    ...     is_imported=True, lock_timestamp=datetime.now(UTC))
+    ...     is_imported=True, lock_timestamp=datetime.now(pytz.UTC))
 
 The statistics change to reflect that the message is not available
 in upstream anymore, so it's now noted as newly translated in Launchpad.
@@ -353,7 +389,7 @@
 
     >>> new_message = potmsgset.updateTranslation(
     ...     pofile_es, no_priv, {0: u'test string'},
-    ...     is_imported=False, lock_timestamp=datetime.now(UTC))
+    ...     is_imported=False, lock_timestamp=datetime.now(pytz.UTC))
 
 This message is not made current or marked as imported.
 
@@ -391,7 +427,7 @@
 
     >>> new_message = potmsgset.updateTranslation(
     ...     pofile_es, foobar, {0: u'test string'},
-    ...     is_imported=False, lock_timestamp=datetime.now(UTC))
+    ...     is_imported=False, lock_timestamp=datetime.now(pytz.UTC))
 
 We don't have any suggestion newer than the new active translation
 
@@ -500,12 +536,12 @@
     2
     >>> message = plural_potmsgset.updateTranslation(pofile_es, foobar,
     ...     {0: u'foo %d', 1: None}, is_imported=False,
-    ...     lock_timestamp=datetime.now(UTC))
+    ...     lock_timestamp=datetime.now(pytz.UTC))
     >>> message.is_complete
     False
     >>> message = plural_potmsgset.updateTranslation(pofile_es, foobar,
     ...     {0: None}, is_imported=False,
-    ...     lock_timestamp=datetime.now(UTC))
+    ...     lock_timestamp=datetime.now(pytz.UTC))
     >>> message.is_complete
     False
 
@@ -537,7 +573,7 @@
 
     >>> message_with_two_forms = pm_potmsgset.updateTranslation(
     ...     pm_translation, pm_template.owner, ['%d fu', '%d fuitl'],
-    ...     is_imported=False, lock_timestamp=datetime.now(UTC))
+    ...     is_imported=False, lock_timestamp=datetime.now(pytz.UTC))
 
 When an otherwise identical translation with three comes along, the
 third form is ignored because it falls outside the current 2 forms.
@@ -546,7 +582,7 @@
     >>> message_with_three_forms = pm_potmsgset.updateTranslation(
     ...     pm_translation, pm_template.owner,
     ...     ['%d fu', '%d fuitl', '%d fuitlx'], is_imported=False,
-    ...     lock_timestamp=datetime.now(UTC))
+    ...     lock_timestamp=datetime.now(pytz.UTC))
     >>> message_with_three_forms == message_with_two_forms
     True
 
@@ -560,7 +596,7 @@
     >>> message_with_three_forms = pm_potmsgset.updateTranslation(
     ...     pm_translation, pm_template.owner,
     ...     ['%d fu', '%d fuitl', '%d fuitlx'], is_imported=False,
-    ...     lock_timestamp=datetime.now(UTC))
+    ...     lock_timestamp=datetime.now(pytz.UTC))
     >>> message_with_three_forms == message_with_two_forms
     False
 
@@ -575,7 +611,7 @@
 
     >>> message_with_one_form = pm_potmsgset.updateTranslation(
     ...     pm_translation, pm_template.owner, ['%d fu'],
-    ...     is_imported=False, lock_timestamp=datetime.now(UTC))
+    ...     is_imported=False, lock_timestamp=datetime.now(pytz.UTC))
     WARNING:...Translation for POTMsgSet ... into 'zap' matches
     2 existing translations.
 
@@ -591,17 +627,16 @@
 This method tells us whether the active translation was reviewed after
 the given timestamp.
 
-    >>> from pytz import UTC
     >>> translationmessage = TranslationMessage.get(2)
     >>> potmsgset = translationmessage.potmsgset
     >>> pofile = translationmessage.pofile
     >>> translationmessage.date_reviewed.isoformat()
     '2005-04-07T13:19:17.601068+00:00'
     >>> potmsgset.isTranslationNewerThan(pofile,
-    ...     datetime(2004, 11, 30, 7, 0, 0, tzinfo=UTC))
+    ...     datetime(2004, 11, 30, 7, 0, 0, tzinfo=pytz.UTC))
     True
     >>> potmsgset.isTranslationNewerThan(pofile,
-    ...     datetime(2006, 11, 30, 7, 0, 0, tzinfo=UTC))
+    ...     datetime(2006, 11, 30, 7, 0, 0, tzinfo=pytz.UTC))
     False
 
 
@@ -951,7 +986,7 @@
 
     >>> new_translation = potmsgset.updateTranslation(pofile_es_variant,
     ...     carlos, {0: u'test variant translation\n'},
-    ...     is_imported=True, lock_timestamp=datetime.now(UTC))
+    ...     is_imported=True, lock_timestamp=datetime.now(pytz.UTC))
 
 A new translation is saved in the PO file.
 
@@ -1007,7 +1042,7 @@
 
     >>> new_translation = translator_credits.updateTranslation(spanish_pofile,
     ...     carlos, {0: u'Some Translator'}, is_imported=True,
-    ...     lock_timestamp=datetime.now(UTC))
+    ...     lock_timestamp=datetime.now(pytz.UTC))
 
     >>> current = translator_credits.getCurrentTranslationMessage(
     ...     alsa_potemplate, spanish)

=== modified file 'lib/lp/translations/interfaces/potmsgset.py'
--- lib/lp/translations/interfaces/potmsgset.py	2009-08-25 20:15:38 +0000
+++ lib/lp/translations/interfaces/potmsgset.py	2009-10-21 10:17:14 +0000
@@ -4,7 +4,8 @@
 # pylint: disable-msg=E0211,E0213
 
 from zope.interface import Interface, Attribute
-from zope.schema import Bool, Int, Object, Text
+from zope.schema import Bool, Choice, Int, Object, Text
+from lazr.enum import EnumeratedType, Item
 
 from canonical.launchpad import _
 from lp.translations.interfaces.pomsgid import IPOMsgID
@@ -15,9 +16,38 @@
     'IPOTMsgSet',
     'BrokenTextError',
     'POTMsgSetInIncompatibleTemplatesError',
+    'TranslationCreditsType',
     ]
 
 
+class TranslationCreditsType(EnumeratedType):
+    """Identify a POTMsgSet as translation credits."""
+
+    NOT_CREDITS = Item("""
+        Not a translation credits message
+
+        This is a standard msgid and not translation credits.
+        """)
+
+    GNOME = Item("""
+        Gnome credits message
+
+        How they do them in Gnome.
+        """)
+
+    KDE_EMAILS = Item("""
+        KDE emails credits message
+
+        How they do them in KDE for translator emails.
+        """)
+
+    KDE_NAMES = Item("""
+        KDE names credits message
+
+        How they do them in KDE for translator names.
+        """)
+
+
 class BrokenTextError(ValueError):
     """Exception raised when we detect values on a text that aren't valid."""
 
@@ -251,6 +281,11 @@
     is_translation_credit = Attribute(
         """Whether this is a message set for crediting translators.""")
 
+    translation_credits_type = Choice(
+        title=u"The type of translation credit of this message.",
+        required=True,
+        vocabulary = TranslationCreditsType)
+
     def makeHTMLID(suffix=None):
         """Unique name for this `POTMsgSet` for use in HTML element ids.
 

=== modified file 'lib/lp/translations/model/pofile.py'
--- lib/lp/translations/model/pofile.py	2009-09-09 11:04:58 +0000
+++ lib/lp/translations/model/pofile.py	2009-10-21 10:17:14 +0000
@@ -30,20 +30,16 @@
 from canonical.database.sqlbase import (
     SQLBase, flush_database_updates, quote, quote_like, sqlvalues)
 from canonical.launchpad import helpers
-from lp.translations.utilities.rosettastats import RosettaStats
-from lp.registry.interfaces.person import validate_public_person
-from lp.translations.model.potmsgset import POTMsgSet
-from lp.translations.model.translationimportqueue import (
-    collect_import_info)
-from lp.translations.model.translationmessage import (
-    TranslationMessage, make_plurals_sql_fragment)
-from lp.translations.model.translationtemplateitem import (
-    TranslationTemplateItem)
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.launchpad.webapp.interfaces import (
     IStoreSelector, MAIN_STORE, MASTER_FLAVOR)
+from canonical.launchpad.webapp.publisher import canonical_url
+from canonical.librarian.interfaces import ILibrarianClient
+from lp.registry.interfaces.person import validate_public_person
+from lp.translations.utilities.rosettastats import RosettaStats
 from lp.translations.interfaces.pofile import IPOFile, IPOFileSet
-from lp.translations.interfaces.potmsgset import BrokenTextError
+from lp.translations.interfaces.potmsgset import (
+    BrokenTextError, TranslationCreditsType)
 from lp.translations.interfaces.translationcommonformat import (
     ITranslationFileData)
 from lp.translations.interfaces.translationexporter import (
@@ -61,11 +57,16 @@
 from lp.translations.interfaces.translationsperson import (
     ITranslationsPerson)
 from lp.translations.interfaces.translations import TranslationConstants
+from lp.translations.model.potmsgset import POTMsgSet
+from lp.translations.model.translationimportqueue import (
+    collect_import_info)
 from lp.translations.model.translatablemessage import TranslatableMessage
+from lp.translations.model.translationmessage import (
+    TranslationMessage, make_plurals_sql_fragment)
+from lp.translations.model.translationtemplateitem import (
+    TranslationTemplateItem)
 from lp.translations.utilities.translation_common_format import (
     TranslationMessageData)
-from canonical.launchpad.webapp.publisher import canonical_url
-from canonical.librarian.interfaces import ILibrarianClient
 
 from storm.expr import And, Join, LeftJoin, Or, SQL
 from storm.info import ClassAlias
@@ -520,17 +521,17 @@
         """See `IPOFile`."""
         LP_CREDIT_HEADER = u'Launchpad Contributions:'
         SPACE = u' '
-        msgid = potmsgset.singular_text
-        assert potmsgset.is_translation_credit, (
+        credits_type = potmsgset.translation_credits_type
+        assert credits_type != TranslationCreditsType.NOT_CREDITS, (
             "Calling prepareTranslationCredits on a message with "
-            "msgid '%s'." % msgid)
+            "msgid '%s'." % potmsgset.singular_text)
         imported = potmsgset.getImportedTranslationMessage(
             self.potemplate, self.language)
         if imported is None:
             text = None
         else:
             text = imported.translations[0]
-        if msgid in [u'_: EMAIL OF TRANSLATORS\nYour emails', u'Your emails']:
+        if credits_type == TranslationCreditsType.KDE_EMAILS:
             emails = []
             if text is not None:
                 emails.append(text)
@@ -547,7 +548,7 @@
                 else:
                     emails.append(preferred_email.email)
             return u','.join(emails)
-        elif msgid in [u'_: NAME OF TRANSLATORS\nYour names', u'Your names']:
+        elif credits_type == TranslationCreditsType.KDE_NAMES:
             names = []
             
             if text is not None:
@@ -561,9 +562,7 @@
                 contributor.displayname
                 for contributor in self.contributors])
             return u','.join(names)
-        elif (msgid in [u'translation-credits',
-                        u'translator-credits',
-                        u'translator_credits']):
+        elif credits_type == TranslationCreditsType.GNOME:
             if len(list(self.contributors)):
                 if text is None:
                     text = u''
@@ -584,7 +583,7 @@
         else:
             raise AssertionError(
                 "Calling prepareTranslationCredits on a message with "
-                "msgid '%s'." % (msgid))
+                "unknown credits type '%s'." % credits_type.title)
 
     def canEditTranslations(self, person):
         """See `IPOFile`."""

=== modified file 'lib/lp/translations/model/potmsgset.py'
--- lib/lp/translations/model/potmsgset.py	2009-08-25 20:15:38 +0000
+++ lib/lp/translations/model/potmsgset.py	2009-10-21 10:17:14 +0000
@@ -19,13 +19,16 @@
 from canonical.database.constants import DEFAULT, UTC_NOW
 from canonical.database.sqlbase import cursor, quote, SQLBase, sqlvalues
 from canonical.launchpad import helpers
+from canonical.launchpad.helpers import shortlist
 from lp.translations.model.translationmessage import (
     make_plurals_sql_fragment)
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
+from lp.translations.interfaces.pofile import IPOFileSet
 from lp.translations.interfaces.potmsgset import (
     BrokenTextError,
     IPOTMsgSet,
-    POTMsgSetInIncompatibleTemplatesError)
+    POTMsgSetInIncompatibleTemplatesError,
+    TranslationCreditsType)
 from lp.translations.interfaces.translationfileformat import (
     TranslationFileFormat)
 from lp.translations.interfaces.translationimporter import (
@@ -35,8 +38,6 @@
     TranslationConflict,
     TranslationValidationStatus)
 from lp.translations.interfaces.translations import TranslationConstants
-from canonical.launchpad.helpers import shortlist
-from lp.translations.interfaces.pofile import IPOFileSet
 from lp.translations.model.pomsgid import POMsgID
 from lp.translations.model.potranslation import POTranslation
 from lp.translations.model.translationmessage import (
@@ -47,20 +48,24 @@
 
 
 # Msgids that indicate translation credit messages, and their
-# contexts.
+# contexts and type.
 credit_message_ids = {
     # Regular gettext credits messages.
-    u'translation-credits': None,
-    u'translator-credits': None,
-    u'translator_credits': None,
+    u'translation-credits': (None, TranslationCreditsType.GNOME),
+    u'translator-credits': (None, TranslationCreditsType.GNOME),
+    u'translator_credits': (None, TranslationCreditsType.GNOME),
 
     # KDE credits messages.
-    u'Your emails': u'EMAIL OF TRANSLATORS',
-    u'Your names': u'NAME OF TRANSLATORS',
+    u'Your emails': 
+        (u'EMAIL OF TRANSLATORS', TranslationCreditsType.KDE_EMAILS),
+    u'Your names':
+        (u'NAME OF TRANSLATORS', TranslationCreditsType.KDE_NAMES),
 
     # Old KDE credits messages.
-    u'_: EMAIL OF TRANSLATORS\nYour emails': None,
-    u'_: NAME OF TRANSLATORS\nYour names': None,
+    u'_: EMAIL OF TRANSLATORS\nYour emails':
+        (None, TranslationCreditsType.KDE_EMAILS),
+    u'_: NAME OF TRANSLATORS\nYour names': 
+        (None, TranslationCreditsType.KDE_NAMES),
     }
 
 
@@ -999,13 +1004,20 @@
     @property
     def is_translation_credit(self):
         """See `IPOTMsgSet`."""
-        # msgid_singular.msgid is pre-joined everywhere where
-        # is_translation_credit is used
+        credit_type = self.translation_credits_type
+        return credit_type != TranslationCreditsType.NOT_CREDITS
+
+    @property
+    def translation_credits_type(self):
+        """See `IPOTMsgSet`."""
         if self.msgid_singular.msgid not in credit_message_ids:
-            return False
+            return TranslationCreditsType.NOT_CREDITS
 
-        expected_context = credit_message_ids[self.msgid_singular.msgid]
-        return expected_context is None or (self.context == expected_context)
+        expected_context, credits_type = (
+            credit_message_ids[self.msgid_singular.msgid])
+        if expected_context is None or (self.context == expected_context):
+            return credits_type
+        return TranslationCreditsType.NOT_CREDITS
 
     def makeHTMLID(self, suffix=None):
         """See `IPOTMsgSet`."""