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

Thanks for the review, and glad you enjoyed the MP. :)

> > === 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?

Personal preference. But I do think it's sensible for free-form text where an apostrophe would be perfectly appropriate. IIRC one of these strings even went so far as to backslash-escape an apostrophe. For God's sake, why!?

Personally I use single quotes (to minimize "ink on the page") for identifiers and such, where an apostrophe would not be appropriate, or for empty strings and things close to it.

> > === 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

> > +
> > + # 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?

Pretty much, yes: "project" and "Ubuntu." A TranslationMessage is selected as current on the "project" side if it has the is_current_upstream flag set, and on the Ubuntu side if it has the is_current_ubuntu flag set. See lp.translations.interfaces.side.

> > + """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.

I don't see a problem with calling it on an untranslated message if it makes the calling code simpler. It's just that there's nothing to do in that case: things will already be in the desired state.

Jeroen