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

No idea how I managed to post my unfinished reply by accident just now, but I'll just continue where I left off.

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

Again rather not in this phase, where we want to be sure we can tweak things without breaking them. Once we start refactoring, the Z probably becomes a no-op anyway.

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

Alright, I've made the message clearer.

> > +
> > + 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.... ;-)

I'm not sure what you mean. As you know (and as mentioned several times in the initial comments for this MP) there is a separate branch with exhaustive tests.

Jeroen