-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 > === modified file 'database/schema/security.cfg' OK > === modified file 'lib/lp/translations/interfaces/potmsgset.py' > --- lib/lp/translations/interfaces/potmsgset.py 2010-06-09 14:17:32 +0000 > +++ lib/lp/translations/interfaces/potmsgset.py 2010-06-17 09:25:53 +0000 > @@ -214,6 +214,10 @@ > force_edition_rights=False, allow_credits=False): > """Update or create a translation message using `new_translations`. > > + This method is Launchpad Translations's sliderule: it does > + everything, nobody fully understands it all, and we intend to > + replace it with a range of less flexible tools. > + > :param pofile: a `POFile` to add `new_translations` to. > :param submitter: author of the translations. > :param new_translations: a dictionary of plural forms, with the > @@ -261,6 +265,20 @@ > If a translation conflict is detected, TranslationConflict is raised. > """ > > + def setCurrentTranslation(pofile, submitter, translations, origin, > + translation_side, share_with_other_side=False): > + """Set the message's translation in Ubuntu, or upstream, or both. > + > + :param pofile: > + :param submitter: > + :param translations: > + :param origin: > + :param translation_side: a `TranslationSide` value. > + :param share_with_other_side: > + """ > + # XXX: Check signature before completing branch. > + # XXX: Update docstring. So, these XXX are supposed to be gone before we land the feature, right? > + > def resetCurrentTranslation(pofile, lock_timestamp): > """Reset the currently used translation. > > === modified file 'lib/lp/translations/interfaces/translations.py' OK > === modified file 'lib/lp/translations/model/potmsgset.py' > --- lib/lp/translations/model/potmsgset.py 2010-06-10 11:24:51 +0000 > +++ lib/lp/translations/model/potmsgset.py 2010-06-17 09:25:53 +0000 > @@ -4,11 +4,16 @@ > # pylint: disable-msg=E0611,W0212 > > __metaclass__ = type > -__all__ = ['POTMsgSet'] > +__all__ = [ > + 'make_translation_side_message_traits', > + 'POTMsgSet', > + ] > + > > import datetime > import logging > import pytz > +import re > > from zope.interface import implements > from zope.component import getUtility > @@ -41,7 +46,8 @@ > RosettaTranslationOrigin, > TranslationConflict, > TranslationValidationStatus) > -from lp.translations.interfaces.translations import TranslationConstants > +from lp.translations.interfaces.translations import ( > + TranslationConstants, TranslationSide) > from lp.translations.model.pomsgid import POMsgID > from lp.translations.model.potranslation import POTranslation > from lp.translations.model.translationmessage import ( > @@ -79,6 +85,91 @@ > u'credits are counted as translated.') > > > +class TranslationSideMessageTraits: > + """Dealing with a `POTMsgSet` on either `TranslationSide`. > + > + Encapsulates primitives that depend on translation side: finding the > + message that is current on the given side, checking the flag that > + says whether a message is current on this side, setting or clearing > + the flag, and providing the same capabilities for the other side. > + > + For an introduction to the Traits pattern, see > + http://www.cantrip.org/traits.html > + """ Traits! Cool concept. I had seen them in the STL but never knew about the concept behind them. > + def __init__(self, potmsgset, potemplate=None, language=None, > + variant=None): > + self.potmsgset = potmsgset > + self.potemplate = potemplate > + self.language = language > + self.variant = variant > + > + self._found_incumbent = False > + > + @property > + def incumbent_message(self): > + """Message that currently has the flag.""" > + if not self._found_incumbent: > + self._incumbent = self._getIncumbentMessage() > + self._found_incumbent = True > + return self._incumbent > + > + def getFlag(self, translationmessage): > + """Is this message the current one on this side?""" > + return getattr(translationmessage, self.flag_name) It is not clear, where flag_name comes from. Wouldn't it be helpful to have a "flag_name = None" attribute on the class, maybe with an explaining comment? > + > + def setFlag(self, translationmessage, value): > + """Set or clear a message's "current" flag for this side.""" > + if value == self.getFlag(translationmessage): > + return > + > + if value and self.incumbent_message is not None: > + self.setFlag(self.incumbent_message, False) > + > + setattr(translationmessage, self.flag_name, value) > + self._found_incumbent = False > + > + def _getIncumbentMessage(self): > + """Get the message that is current on this side, if any.""" > + raise NotImplementedError('_getIncumbentMessage') > + > + > +class UpstreamSideTraits(TranslationSideMessageTraits): > + """Traits for upstream translations.""" > + > + side = TranslationSide.UPSTREAM > + > + flag_name = 'is_current_upstream' > + > + def _getIncumbentMessage(self): > + """See `TranslationSideMessageTraits`.""" > + return self.potmsgset.getImportedTranslationMessage( > + self.potemplate, self.language, variant=self.variant) > + > + > +class UbuntuSideTraits(TranslationSideMessageTraits): > + """Traits for Ubuntu translations.""" > + > + side = TranslationSide.UBUNTU > + > + flag_name = 'is_current_ubuntu' > + > + def _getIncumbentMessage(self): > + """See `TranslationSideMessageTraits`.""" > + return self.potmsgset.getCurrentTranslationMessage( > + self.potemplate, self.language, variant=self.variant) > + > + > +def make_translation_side_message_traits(side, potmsgset, potemplate, > + language, variant=None): > + """Create `TranslationSideTraits` object of the appropriate subtype.""" > + ubuntu = UbuntuSideTraits(potmsgset, potemplate, language, variant) > + upstream = UpstreamSideTraits(potmsgset, potemplate, language, variant) > + upstream.other_side = ubuntu > + ubuntu.other_side = upstream A completely undocumented attribute AFICT. I think either the base traits class or the derived classes could use some documentary class attributes. ;-) > + mapping = dict((traits.side, traits) for traits in (ubuntu, upstream)) > + return mapping[side] > + > + > class POTMsgSet(SQLBase): > implements(IPOTMsgSet) > > @@ -232,13 +323,13 @@ > current=True): > """Get a translation message which is either used in > Launchpad (current=True) or in an import (current=False). > - > + > Prefers a diverged message if present. > """ > - # Change the is_current_ubuntu and is_current_upstream conditions > - # with extreme care: they need to match condition specified in > - # indexes, or postgres may not pick them up (in complicated queries, > - # postgres query optimizer sometimes does text-matching of indexes). > + # Change 'is_current IS TRUE' and 'is_imported IS TRUE' conditions Hm, this looks like something is being reverted ... > + # carefully: they need to match condition specified in indexes, > + # or Postgres may not pick them up (in complicated queries, > + # Postgres query optimizer sometimes does text-matching of indexes). > if current: > used_clause = 'is_current_ubuntu IS TRUE' > else: > @@ -522,13 +613,17 @@ > potranslations[pluralform] = None > return potranslations > > - def _findTranslationMessage(self, pofile, potranslations, pluralforms): > - """Find a message for this `pofile`. > - > - The returned message matches exactly the given `translations` strings > - comparing only `pluralforms` of them. > + def _findTranslationMessage(self, pofile, potranslations, > + prefer_shared=True): > + """Find a matching message in this `pofile`. > + > + The returned message matches exactly the given `translations` > + strings (except plural forms not supported by `pofile`, which > + are ignored). > + > :param potranslations: A list of translation strings. > - :param pluralforms: The number of pluralforms to compare. > + :param prefer_shared: Whether to prefer a shared match over a > + diverged one. > """ > clauses = ['potmsgset = %s' % sqlvalues(self), > 'language = %s' % sqlvalues(pofile.language), > @@ -539,7 +634,7 @@ > else: > clauses.append('variant = %s' % sqlvalues(pofile.variant)) > > - for pluralform in range(pluralforms): > + for pluralform in range(pofile.plural_forms): > if potranslations[pluralform] is None: > clauses.append('msgstr%s IS NULL' % sqlvalues(pluralform)) > else: > @@ -547,10 +642,15 @@ > sqlvalues(pluralform, potranslations[pluralform]))) > > remaining_plural_forms = range( > - pluralforms, TranslationConstants.MAX_PLURAL_FORMS) > - > - # Prefer shared messages over diverged ones. > - order = ['potemplate NULLS FIRST'] > + pofile.plural_forms, TranslationConstants.MAX_PLURAL_FORMS) > + > + # Prefer either shared or diverged messages, depending on > + # arguments. > + if prefer_shared: > + order = ['potemplate NULLS FIRST'] > + else: > + order = ['potemplate NULLS LAST'] > + > # Normally at most one message should match. But if there is > # more than one, prefer the one that adds the fewest extraneous > # plural forms. > @@ -564,9 +664,9 @@ > if len(matches) > 0: > if len(matches) > 1: > logging.warn( > - "Translation for POTMsgSet %s into %s " > - "matches %s existing translations." % sqlvalues( > - self, pofile.language.code, len(matches))) > + "Translation for POTMsgSet %s into '%s' " > + "matches %s existing translations.", > + self.id, pofile.language.code, len(matches)) > return matches[0] > else: > return None > @@ -799,7 +899,7 @@ > # of translations. None if there is no such message and needs to be > # created. > matching_message = self._findTranslationMessage( > - pofile, potranslations, pofile.plural_forms) > + pofile, potranslations) > > match_is_upstream = ( > matching_message is not None and > @@ -897,7 +997,7 @@ > potranslations = self._findPOTranslations(new_translations) > > existing_message = self._findTranslationMessage( > - pofile, potranslations, pofile.plural_forms) > + pofile, potranslations) > if existing_message is not None: > return existing_message > > @@ -951,10 +1051,188 @@ > current.reviewer = reviewer > current.date_reviewed = lock_timestamp > > - def setCurrentTranslation(self, pofile, submitter, translations, origin, > - translation_side, share_with_other_side=False): > - """See `IPOTMsgSet`.""" > - > +<<<<<<< TREE > + def setCurrentTranslation(self, pofile, submitter, translations, origin, > + translation_side, share_with_other_side=False): > + """See `IPOTMsgSet`.""" > + So, this is easy to resolve. This has to go away, there is a declaration further down. > +======= > + def _nameMessageStatus(self, message, translation_side_traits): > + """Figure out the decision-matrix status of a message. > + > + This is used in navigating the decision matrix in > + `setCurrentTranslation`. > + """ > + if message is None: > + return 'none' > + elif message.potemplate is None: > + if translation_side_traits.other_side.getFlag(message): > + return 'other_shared' > + else: > + return 'shared' > + else: > + assert message.potemplate is not None, "Confused message state." Your either forgot this in here or it's paranoia. Since the preceding elif checks the exact same condition, this really is not needed. > + return 'diverged' > + > + def _makeTranslationMessage(self, pofile, submitter, translations, origin, > + diverged=False): > + # XXX: Document. > + """.""" > + if diverged: > + potemplate = pofile.potemplate > + else: > + potemplate = None > + > + translation_args = dict( > + ('msgstr%d' % form, translation) > + for form, translation in translations.iteritems() > + ) > + > + return TranslationMessage( > + potmsgset=self, > + potemplate=potemplate, > + pofile=pofile, > + language=pofile.language, > + variant=pofile.variant, > + origin=origin, > + submitter=submitter, > + validation_status=TranslationValidationStatus.OK, > + **translation_args) > + > + def setCurrentTranslation(self, pofile, submitter, translations, origin, > + translation_side, share_with_other_side=False): > + """See `IPOTMsgSet`.""" > + traits = make_translation_side_message_traits( > + translation_side, self, pofile.potemplate, pofile.language, > + variant=pofile.variant) > + > + translations = self._findPOTranslations(translations) > + incumbent_message = traits.incumbent_message > + twin = self._findTranslationMessage( > + pofile, translations, prefer_shared=False) Hm, I am not 100% sure what a twin is. Maybe you could explain that here? > + > + # Summary of the matrix: > + # * If the incumbent message is diverged and we're setting a > + # translation that's already shared: converge. > + # * If the incumbent message is diverged and we're setting a > + # translation that's not already shared: maintain divergence. > + # * If the incumbent message is shared, replace it. > + # * If there is no twin, simply create a new message (shared or > + # diverged depending; see above). > + # * If there is a shared twin, activate it (but also diverge if > + # necessary; see above). > + # * If there is a diverged twin, activate it (and converge it > + # if appropriate; see above). > + # * If there is a twin that's shared on the other side, > + > + # XXX: Steal flag if policy permits and either: Do we still need to agree on this policy? > + # - there is no shared active message on the other side, or > + # - we're returning a shared message. > + > + decision_matrix = { > + 'incumbent_none': { > + 'twin_none': 'Z1*', > + 'twin_shared': 'Z4*', > + 'twin_diverged': 'Z7*', > + 'twin_other_shared': 'Z4', > + }, > + 'incumbent_shared': { > + 'twin_none': 'B1*', > + 'twin_shared': 'B4*', > + 'twin_diverged': 'B7*', > + 'twin_other_shared': 'B4', > + }, > + 'incumbent_diverged': { > + 'twin_none': 'A2', > + 'twin_shared': 'A5', > + 'twin_diverged': 'A4', > + 'twin_other_shared': 'A6', > + }, > + 'incumbent_other_shared': { > + 'twin_none': 'B1+', > + 'twin_shared': 'B4+', > + 'twin_diverged': 'B7+', > + 'twin_other_shared': '', This last one should be '4' instead of special casing it at the end. > + }, > + } > + > + incumbent_state = "incumbent_%s" % self._nameMessageStatus( > + incumbent_message, traits) > + twin_state = "twin_%s" % self._nameMessageStatus(twin, traits) > + > + decisions = decision_matrix[incumbent_state][twin_state] > + file('/tmp/matrix.log','a').write(decisions+'\n') # XXX: DEBUG CODE > + assert re.match('[ABZ]?[124567]?[+*]?$', decisions), ( > + "Bad decision string.") Paranoia! > + > + for character in decisions: > + if character == 'A': > + # Deactivate & converge. > + # There may be an identical shared message. > + traits.setFlag(incumbent_message, False) > + incumbent_message.shareIfPossible() > + elif character == 'B': > + # Deactivate. > + traits.setFlag(incumbent_message, False) > + elif character == 'Z': > + # There is no incumbent message, so do nothing to it. > + assert incumbent_message is None, ( > + "Incorrect Z in decision matrix.") Ok, first you hide this check in _nameMessageStatus, now you pull it out in the open for an assert. Maybe you should trust your matrix more and just put a "pass" in here? > + elif character == '1': > + # Create & activate. > + message = self._makeTranslationMessage( > + pofile, submitter, translations, origin) > + elif character == '2': > + # Create, diverge, activate. > + message = self._makeTranslationMessage( > + pofile, submitter, translations, origin, diverged=True) > + elif character == '4': > + # Activate. > + message = twin > + elif character == '5': > + # If other is a suggestion, diverge and activate. > + # (If not, it's already active and has been unmasked by > + # our deactivating the incumbent). > + message = twin > + if not traits.getFlag(twin): > + assert not traits.other_side.getFlag(twin), ( > + "Decision matrix says '5' for a message that's " > + "active on the other side.") Yes, that is what is done in _nameMessageStatus, so duplicated here. Also, the error message is quite cryptic to an outsider. > + message.potemplate = pofile.potemplate > + elif character == '6': > + # If other is not active, fork a diverged message. > + if traits.getFlag(twin): > + message = twin > + else: > + # The twin is used on the other side, so we can't > + # just reuse it for our diverged message. Create a > + # new one. > + message = self._makeTranslationMessage( > + pofile, submitter, translations, origin, > + diverged=True) > + elif character == '7': > + # Converge & activate. > + message = twin > + message.shareIfPossible() > + elif character == '*': > + if share_with_other_side: > + if traits.other_side.incumbent_message is None: > + traits.other_side.setFlag(message, True) > + elif character == '+': > + if share_with_other_side: > + traits.other_side.setFlag(message, True) > + else: > + raise AssertionError( > + "Bad character in decision string: %s" % character) > + > + if decisions == '': > + message = twin This can go away if you use the '4' as mentioned earlier. > + > + traits.setFlag(message, True) > + > + return message > + > +>>>>>>> MERGE-SOURCE > def resetCurrentTranslation(self, pofile, lock_timestamp): > """See `IPOTMsgSet`.""" > > @@ -1145,7 +1423,7 @@ > # The credits message has a fixed "translator." > translator = getUtility(ILaunchpadCelebrities).rosetta_experts > > - message = self.updateTranslation( > + self.updateTranslation( > pofile, translator, [credits_message_str], > is_current_upstream=False, allow_credits=True, > force_shared=True, force_edition_rights=True, > === modified file 'lib/lp/translations/tests/test_potmsgset.py' The tests look very good although I am sure there could be even more.... ;-) Thanks for doing such a good job. I really like the approach to use the matrix explicitly in the code and also the traits are a great idea. Well done! I will not approve this yet, obviously, because of the text conflicts but I have nothing else that needs serious fixing. review needs-fixing code Cheers, Henning -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAkwaTMsACgkQBT3oW1L17ijACQCdFBkgqS4mFPcJnrYENrq3tqms 2FQAoLBZXCQ5CvV9L4mL21+G133GOe0x =v9bE -----END PGP SIGNATURE-----