Code review comment for lp:~jtv/launchpad/recife-new-resetcurrenttranslation

Heh... I enjoyed reading your MP description :)

And yes, I've been in that position before too... the one case (a transition to new infrastructure) where a bad name is a good one.

I've just got one question below about "the other side". But r=me. And great tests :)

> === modified file 'lib/lp/translations/browser/translationmessage.py'
> --- lib/lp/translations/browser/translationmessage.py 2010-08-23 08:35:29 +0000
> +++ lib/lp/translations/browser/translationmessage.py 2010-08-26 10:24:07 +0000
> @@ -300,16 +299,16 @@
>
> if self.request.method == 'POST':
> if self.user is None:
> - raise UnexpectedFormData, (
> - 'Anonymous users or users who are not accepting our '
> - 'licensing terms cannot do POST submissions.')
> + raise UnexpectedFormData(
> + "Anonymous users or users who are not accepting our "
> + "licensing terms cannot do POST submissions.")

Out of interest, is switching to double-quotes by default a personal
preference or a new LP coding recommendation?

> === modified file 'lib/lp/translations/model/potmsgset.py'
> --- lib/lp/translations/model/potmsgset.py 2010-08-24 11:39:06 +0000
> +++ lib/lp/translations/model/potmsgset.py 2010-08-26 10:24:07 +0000
> @@ -1295,25 +1295,47 @@
>
> return message
>
> - def resetCurrentTranslation(self, pofile, lock_timestamp):
> - """See `IPOTMsgSet`."""
> -
> - assert(lock_timestamp is not None)
> -
> + def old_resetCurrentTranslation(self, pofile, lock_timestamp):
> + """See `POTMsgSet`.
> +
> + This message is OBSOLETE in the Recife feature branch. It's
> + still here only until we replace its one call with the new
> + method.
> + """
> + assert lock_timestamp is not None, "No lock timestamp given."
> current = self.getCurrentTranslationMessage(
> pofile.potemplate, pofile.language)
> -
> - if (current is not None):
> - # Check for translation conflicts and update the required
> - # attributes.
> - self._maybeRaiseTranslationConflict(current, lock_timestamp)
> - current.is_current_ubuntu = False
> - # Converge the current translation only if it is diverged and not
> - # current upstream.
> - is_diverged = current.potemplate is not None
> - if is_diverged and not current.is_current_upstream:
> - current.potemplate = None
> - pofile.date_changed = UTC_NOW
> + if current is None:
> + return

Nice... I prefer guard code like this too :)

> +
> + # Check for translation conflicts and update the required
> + # attributes.
> + self._maybeRaiseTranslationConflict(current, lock_timestamp)
> + current.is_current_ubuntu = False
> + # Converge the current translation only if it is diverged and
> + # not current upstream.
> + if current.is_diverged and not current.is_current_upstream:
> + current.potemplate = None
> + pofile.markChanged()
> +
> + def resetCurrentTranslation(self, pofile, lock_timestamp=None,
> + share_with_other_side=False):

OK, I forgot to mention this on the IFace declaration, but I have no idea what
the other side is (maybe the interface description could say a bit more to
cater for non-translation folk?). From your tests, it looks like it is
upstream/downstream?

> + """See `IPOTMsgSet`."""
> + traits = getUtility(ITranslationSideTraitsSet).getTraits(
> + pofile.potemplate.translation_side)
> + current_message = traits.getCurrentMessage(
> + self, pofile.potemplate, pofile.language)
> +
> + if current_message is None:
> + # Nothing to do here.

Is this an error? If a reset is called on a translation that doesn't yet have
a translation? I'm sure you've thought of that and decided its not, but just in
case.