launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01941
[Merge] lp:~jtv/launchpad/recife-getcurrent-breakage into lp:~launchpad/launchpad/recife
Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/recife-getcurrent-breakage into lp:~launchpad/launchpad/recife.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers): code
= Fix up Recife feature branch =
This patches up an expected problem in the Recife feature branch. In order to be able to make progress on other work, I implemented a quick sketch method POTMsgSet.getCurrentTranslation that would replace some calls to the obsolescent getCurrentTranslationMessage with something that follows the new "Recife" data model. It was just enough to keep me going.
Meanwhile, Danilo started on a more permanent implementation of getCurrentTranslation that would additionally replace getImportedTranslationMessage, an if anything even more obsolescent sibling to getCurrentTranslationMessage. So we ended up with two different implementations of getCurrentTranslation, with slightly different interfaces. Here I patch up uses of mine to use his instead, so that mine can be retired.
I also fix up some test helpers that were meant to abstract away the difference between the two translation "sides" (Ubuntu and upstream) that getCurrentTranslationMessage and getImportedTranslation represent, but were stuck in the "old model" because those two methods are hard-coded to deal with one translation "side" each. Now, calls to getCurrentTranslationMessage should turn into getCurrentTranslation calls where you pass in the "side" of the template you're looking at; a rough equivalent to getImportedTranslation in the new model is to call getCurrentTranslation but choose the other "side."
Fixing up those helpers in turn let me revive a setCurrentTranslation test that we had had to disable. It's the same extensive test scenario (implemented in a base class) that we were running on the Ubuntu side, but not yet on the upstream side. Fixing the helpers made the upstream-side version of the test pass without a hitch. The only change needed in the test was to un-comment it.
You'll also notice a lot of drive-by fixes to relatively recent code. It seems that reviewers have been ignoring our coding standards of late, or been pressured into letting violations slide, or bypassed altogether. We're clearly no longer reinforcing our good habits to the point where they are easy for responsible adults to pick up and maintain. So reviewer, make a difference in this branch—give me hell! :-)
The only real way to test this branch is to run all Translations tests. We'll Q/A it as a whole, once we have the new data model fully supported.
I left one piece of lint in place: a stand-alone comment block annoys our lint checker's blank-line counter. I don't know whether there really is a different preferred style for that, or it's a forgotten corner of the checker, or what. So I left it.
Jeroen
--
https://code.launchpad.net/~jtv/launchpad/recife-getcurrent-breakage/+merge/41045
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/recife-getcurrent-breakage into lp:~launchpad/launchpad/recife.
=== modified file 'lib/lp/translations/interfaces/potmsgset.py'
--- lib/lp/translations/interfaces/potmsgset.py 2010-11-16 16:25:21 +0000
+++ lib/lp/translations/interfaces/potmsgset.py 2010-11-17 13:23:55 +0000
@@ -156,12 +156,6 @@
Diverged messages are preferred.
"""
- def getCurrentTranslation(potemplate, language):
- # XXX JeroenVermeulen 2010-11-16: Stub. To be replaced with
- # Danilo's simultaneous work. If you see both definitions, this
- # is the one you should drop.
- """Retrieve a current `TranslationMessage`."""
-
def getSharedTranslationMessage(language):
"""Returns a shared TranslationMessage."""
@@ -285,11 +279,11 @@
If a translation conflict is detected, TranslationConflict is raised.
"""
- def getCurrentTranslation(potemplate, language, side=None):
+ def getCurrentTranslation(potemplate, language, side):
"""Get a current translation message.
:param potemplate: An `IPOTemplate` to look up a translation for.
- If it's None, returns a shared translation.
+ If it's None, ignore diverged translations.
:param language: translation should be to this `ILanguage`.
:param side: translation side to look at.
"""
=== modified file 'lib/lp/translations/model/potmsgset.py'
--- lib/lp/translations/model/potmsgset.py 2010-11-16 18:00:07 +0000
+++ lib/lp/translations/model/potmsgset.py 2010-11-17 13:23:55 +0000
@@ -32,6 +32,7 @@
)
from zope.component import getUtility
from zope.interface import implements
+from zope.security.proxy import removeSecurityProxy
from canonical.config import config
from canonical.database.constants import (
@@ -334,24 +335,23 @@
def getCurrentTranslation(self, potemplate, language, side):
"""See `IPOTMsgSet`."""
-
- from zope.security.proxy import removeSecurityProxy
assert side is not None, "Translation side must be specified."
-
traits = getUtility(ITranslationSideTraitsSet).getTraits(side)
- clauses = [removeSecurityProxy(
- traits.getFlag(TranslationMessage)) == True]
+ flag = removeSecurityProxy(traits.getFlag(TranslationMessage))
+
+ clauses = [
+ flag == True,
+ TranslationMessage.potmsgsetID == self.id,
+ TranslationMessage.languageID == language.id,
+ ]
if potemplate is None:
# Look only for a shared translation.
clauses.append(TranslationMessage.potemplate == None)
else:
- clauses.append(
- Or(TranslationMessage.potemplate == None,
- TranslationMessage.potemplateID == potemplate.id))
- clauses.extend([
- TranslationMessage.potmsgsetID == self.id,
- TranslationMessage.languageID == language.id])
+ clauses.append(Or(
+ TranslationMessage.potemplate == None,
+ TranslationMessage.potemplateID == potemplate.id))
# Return a diverged translation if it exists, and fall back
# to the shared one otherwise.
@@ -374,7 +374,8 @@
"msgstr%(form)d IS NOT NULL", "OR")
query += " AND (%s)" % msgstr_clause
if include_dismissed != include_unreviewed:
- current = self.getCurrentTranslation(potemplate, language)
+ current = self.getCurrentTranslation(
+ potemplate, language, potemplate.translation_side)
if current is not None:
if current.date_reviewed is None:
comparing_date = current.date_created
@@ -1057,8 +1058,10 @@
def dismissAllSuggestions(self, pofile, reviewer, lock_timestamp):
"""See `IPOTMsgSet`."""
- current = self.getCurrentTranslation(
- pofile.potemplate, pofile.language)
+ template = pofile.potemplate
+ language = pofile.language
+ side = template.translation_side
+ current = self.getCurrentTranslation(template, language, side)
if current is None:
# Create or activate an empty translation message.
current = self.setCurrentTranslation(
@@ -1068,7 +1071,7 @@
# Check for translation conflicts and update review fields.
self._maybeRaiseTranslationConflict(current, lock_timestamp)
current.markReviewed(reviewer, lock_timestamp)
- assert self.getCurrentTranslation(pofile.potemplate, pofile.language)
+ assert self.getCurrentTranslation(template, language, side)
def _nameMessageStatus(self, message, translation_side_traits):
"""Figure out the decision-matrix status of a message.
=== modified file 'lib/lp/translations/model/translatablemessage.py'
--- lib/lp/translations/model/translatablemessage.py 2010-11-16 08:10:16 +0000
+++ lib/lp/translations/model/translatablemessage.py 2010-11-17 13:23:55 +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).
"""Implementation for `ITranslatableMessage`."""
@@ -45,7 +45,7 @@
self.language = pofile.language
self._current_translation = self.potmsgset.getCurrentTranslation(
- self.potemplate, self.language)
+ self.potemplate, self.language, self.potemplate.translation_side)
@property
def is_obsolete(self):
@@ -102,20 +102,20 @@
def getAllSuggestions(self):
"""See `ITranslatableMessage`"""
return self.potmsgset.getLocalTranslationMessages(
- self.potemplate, self.language,
- include_dismissed=True, include_unreviewed=True)
+ self.potemplate, self.language,
+ include_dismissed=True, include_unreviewed=True)
def getUnreviewedSuggestions(self):
"""See `ITranslatableMessage`"""
return self.potmsgset.getLocalTranslationMessages(
- self.potemplate, self.language,
- include_dismissed=False, include_unreviewed=True)
+ self.potemplate, self.language,
+ include_dismissed=False, include_unreviewed=True)
def getDismissedSuggestions(self):
"""See `ITranslatableMessage`"""
return self.potmsgset.getLocalTranslationMessages(
- self.potemplate, self.language,
- include_dismissed=True, include_unreviewed=False)
+ self.potemplate, self.language,
+ include_dismissed=True, include_unreviewed=False)
def getExternalTranslations(self):
"""See `ITranslatableMessage`"""
@@ -129,6 +129,5 @@
def dismissAllSuggestions(self, reviewer, lock_timestamp):
"""See `ITranslatableMessage`"""
- self.potmsgset.dismissAllSuggestions(self.pofile,
- reviewer, lock_timestamp)
-
+ self.potmsgset.dismissAllSuggestions(
+ self.pofile, reviewer, lock_timestamp)
=== modified file 'lib/lp/translations/tests/helpers.py'
--- lib/lp/translations/tests/helpers.py 2010-08-23 08:41:03 +0000
+++ lib/lp/translations/tests/helpers.py 2010-11-17 13:23:55 +0000
@@ -12,8 +12,10 @@
Or,
)
from storm.store import Store
+from zope.component import getUtility
from zope.security.proxy import removeSecurityProxy
+from lp.translations.interfaces.side import ITranslationSideTraitsSet
from lp.translations.interfaces.translationmessage import (
RosettaTranslationOrigin,
TranslationValidationStatus,
@@ -113,17 +115,22 @@
* list of all other diverged translations (not including the one
diverged in `pofile`) or an empty list if there are none.
"""
- current_shared = potmsgset.getCurrentTranslationMessage(
- None, pofile.language)
- current_diverged = potmsgset.getCurrentTranslationMessage(
- pofile.potemplate, pofile.language)
+ template = pofile.potemplate
+ language = pofile.language
+ side = template.translation_side
+
+ current_shared = potmsgset.getCurrentTranslation(None, language, side)
+ current_diverged = potmsgset.getCurrentTranslation(
+ template, language, side)
if current_diverged is not None and not current_diverged.is_diverged:
current_diverged = None
- other_shared = potmsgset.getImportedTranslationMessage(
- None, pofile.language)
- other_diverged = potmsgset.getImportedTranslationMessage(
- pofile.potemplate, pofile.language)
+ traits = getUtility(ITranslationSideTraitsSet).getTraits(side)
+ other_side = traits.other_side_traits.side
+
+ other_shared = potmsgset.getCurrentTranslation(None, language, other_side)
+ other_diverged = potmsgset.getCurrentTranslation(
+ template, language, other_side)
assert other_diverged is None or other_diverged.potemplate is None, (
"There is a diverged 'other' translation for "
"this same template, which should be impossible.")
@@ -133,7 +140,7 @@
diverged = []
for suggestion in all_used:
if ((suggestion.potemplate is not None and
- suggestion.potemplate != pofile.potemplate) and
+ suggestion.potemplate != template) and
(suggestion.is_current_ubuntu or
suggestion.is_current_upstream)):
# It's diverged for another template and current somewhere.
=== modified file 'lib/lp/translations/tests/test_potmsgset.py'
--- lib/lp/translations/tests/test_potmsgset.py 2010-11-16 16:25:21 +0000
+++ lib/lp/translations/tests/test_potmsgset.py 2010-11-17 13:23:55 +0000
@@ -29,7 +29,10 @@
POTMsgSetInIncompatibleTemplatesError,
TranslationCreditsType,
)
-from lp.translations.interfaces.side import ITranslationSideTraitsSet
+from lp.translations.interfaces.side import (
+ ITranslationSideTraitsSet,
+ TranslationSide,
+ )
from lp.translations.interfaces.translationfileformat import (
TranslationFileFormat,
)
@@ -37,7 +40,6 @@
RosettaTranslationOrigin,
TranslationConflict,
)
-from lp.translations.interfaces.side import TranslationSide
from lp.translations.model.translationmessage import DummyTranslationMessage
@@ -870,7 +872,8 @@
self.pofile, self.factory.makePerson(), self.now())
transaction.commit()
current = self.potmsgset.getCurrentTranslation(
- self.potemplate, self.pofile.language)
+ self.potemplate, self.pofile.language,
+ self.potemplate.translation_side)
self.assertNotEqual(None, current)
self.assertEqual([None], current.translations)
# All suggestions are gone.
@@ -1718,10 +1721,9 @@
def test_no_translation(self):
# getCurrentTranslation returns None when there's no translation.
pofile, potmsgset = self._makePOFileAndPOTMsgSet()
-
- self.assertIs(None,
- potmsgset.getCurrentTranslation(
- pofile.potemplate, pofile.language, self.this_side))
+ current = potmsgset.getCurrentTranslation(
+ pofile.potemplate, pofile.language, self.this_side)
+ self.assertIs(None, current)
def test_basic_get(self):
# getCurrentTranslation gets the current translation
@@ -1733,19 +1735,16 @@
message = potmsgset.setCurrentTranslation(
pofile, pofile.potemplate.owner, translations, origin)
- self.assertEqual(message,
- potmsgset.getCurrentTranslation(
- pofile.potemplate, pofile.language,
- self.this_side))
+ current = potmsgset.getCurrentTranslation(
+ pofile.potemplate, pofile.language, self.this_side)
+ self.assertEqual(message, current)
- def test_assertion_on_bad_parameters(self):
- # When no potemplate is passed in to getCurrentTranslation,
- # and no explicit side is selected, AssertionError is raised.
+ def test_requires_side(self):
pofile, potmsgset = self._makePOFileAndPOTMsgSet()
-
- self.assertRaises(AssertionError,
- potmsgset.getCurrentTranslation,
- None, pofile.language, None)
+ self.assertRaises(
+ AssertionError,
+ potmsgset.getCurrentTranslation,
+ pofile.potemplate, pofile.language, None)
def test_other_languages_ignored(self):
# getCurrentTranslation never returns a translation for another
@@ -1760,9 +1759,9 @@
pofile_other_language, pofile.potemplate.owner,
translations, origin)
- self.assertIs(None,
- potmsgset.getCurrentTranslation(
- pofile.potemplate, pofile.language, self.this_side))
+ current = potmsgset.getCurrentTranslation(
+ pofile.potemplate, pofile.language, self.this_side)
+ self.assertIs(None, current)
def test_other_diverged_no_translation(self):
# getCurrentTranslation gets the current upstream translation
@@ -1778,9 +1777,9 @@
suggestion.approveAsDiverged(
pofile_other, pofile_other.potemplate.owner)
- self.assertIs(None,
- potmsgset.getCurrentTranslation(
- pofile.potemplate, pofile.language, self.this_side))
+ current = potmsgset.getCurrentTranslation(
+ pofile.potemplate, pofile.language, self.this_side)
+ self.assertIs(None, current)
def test_other_side(self):
# getCurrentTranslation gets the current translation
@@ -1802,14 +1801,15 @@
pofile_other, pofile_other.potemplate.owner,
translations_other, origin)
- self.assertEquals(current_translation,
- potmsgset.getCurrentTranslation(
- pofile_other.potemplate, pofile_other.language,
- self.this_side))
- self.assertEquals(other_translation,
- potmsgset.getCurrentTranslation(
- pofile.potemplate, pofile.language,
- self.other_side))
+ self.assertEquals(
+ current_translation,
+ potmsgset.getCurrentTranslation(
+ pofile_other.potemplate, pofile_other.language,
+ self.this_side))
+ self.assertEquals(
+ other_translation,
+ potmsgset.getCurrentTranslation(
+ pofile.potemplate, pofile.language, self.other_side))
def test_prefers_diverged(self):
# getCurrentTranslation prefers a diverged translation if
@@ -1828,10 +1828,9 @@
pofile, pofile.potemplate.owner, translations_diverged)
diverged_message.approveAsDiverged(pofile, pofile.potemplate.owner)
- self.assertEquals(diverged_message,
- potmsgset.getCurrentTranslation(
- pofile.potemplate, pofile.language,
- self.this_side))
+ current = potmsgset.getCurrentTranslation(
+ pofile.potemplate, pofile.language, self.this_side)
+ self.assertEquals(diverged_message, current)
def test_shared_when_requested(self):
# getCurrentTranslation returns a shared translation even with
@@ -1850,9 +1849,9 @@
pofile, pofile.potemplate.owner, translations_diverged)
diverged_message.approveAsDiverged(pofile, pofile.potemplate.owner)
- self.assertEquals(shared_message,
- potmsgset.getCurrentTranslation(
- None, pofile.language, self.this_side))
+ current = potmsgset.getCurrentTranslation(
+ None, pofile.language, self.this_side)
+ self.assertEquals(shared_message, current)
class TestGetCurrentTranslationForUpstreams(BaseTestGetCurrentTranslation,
=== modified file 'lib/lp/translations/tests/test_setcurrenttranslation.py'
--- lib/lp/translations/tests/test_setcurrenttranslation.py 2010-08-23 08:41:03 +0000
+++ lib/lp/translations/tests/test_setcurrenttranslation.py 2010-11-17 13:23:55 +0000
@@ -1152,37 +1152,37 @@
# getCurrentTranslation and getImportedTranslation "change sides" based
# on the current template: lp:~danilo/launchpad/use-upstream-translations.
-#class TestSetCurrentTranslation_Upstream(SetCurrentTranslationTestMixin,
-# TestCaseWithFactory):
-#
-# layer = ZopelessDatabaseLayer
-#
-# def setUp(self):
-# super(TestSetCurrentTranslation_Upstream, self).setUp()
-# series = self.factory.makeProductSeries()
-# sharing_series = self.factory.makeProductSeries(
-# product=series.product)
-# potemplate = self.factory.makePOTemplate(productseries=series)
-# sharing_potemplate = self.factory.makePOTemplate(
-# productseries=sharing_series, name=potemplate.name)
-# self.pofile = self.factory.makePOFile('sr', potemplate=potemplate,
-# create_sharing=True)
-#
-# # A POFile in the same context as self.pofile, used for diverged
-# # translations.
-# self.diverging_pofile = sharing_potemplate.getPOFileByLang(
-# self.pofile.language.code)
-#
-# # A POFile in a different context from self.pofile and
-# # self.diverging_pofile.
-# ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
-# sourcepackagename = self.factory.makeSourcePackageName()
-# ubuntu_template = self.factory.makePOTemplate(
-# distroseries=ubuntu.currentseries,
-# sourcepackagename=sourcepackagename)
-# self.other_pofile = self.factory.makePOFile(
-# potemplate=ubuntu_template,
-# language_code=self.pofile.language.code)
-#
-# self.potmsgset = self.factory.makePOTMsgSet(potemplate=potemplate,
-# sequence=1)
+class TestSetCurrentTranslation_Upstream(SetCurrentTranslationTestMixin,
+ TestCaseWithFactory):
+
+ layer = ZopelessDatabaseLayer
+
+ def setUp(self):
+ super(TestSetCurrentTranslation_Upstream, self).setUp()
+ series = self.factory.makeProductSeries()
+ sharing_series = self.factory.makeProductSeries(
+ product=series.product)
+ potemplate = self.factory.makePOTemplate(productseries=series)
+ sharing_potemplate = self.factory.makePOTemplate(
+ productseries=sharing_series, name=potemplate.name)
+ self.pofile = self.factory.makePOFile(
+ 'sr', potemplate=potemplate, create_sharing=True)
+
+ # A POFile in the same context as self.pofile, used for diverged
+ # translations.
+ self.diverging_pofile = sharing_potemplate.getPOFileByLang(
+ self.pofile.language.code)
+
+ # A POFile in a different context from self.pofile and
+ # self.diverging_pofile.
+ ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
+ sourcepackagename = self.factory.makeSourcePackageName()
+ ubuntu_template = self.factory.makePOTemplate(
+ distroseries=ubuntu.currentseries,
+ sourcepackagename=sourcepackagename)
+ self.other_pofile = self.factory.makePOFile(
+ potemplate=ubuntu_template,
+ language_code=self.pofile.language.code)
+
+ self.potmsgset = self.factory.makePOTMsgSet(
+ potemplate=potemplate, sequence=1)
