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