Code review comment for lp://staging/~jtv/launchpad/recife-add-suggestion

> === modified file 'lib/lp/translations/model/potmsgset.py'
> --- lib/lp/translations/model/potmsgset.py 2010-03-23 15:39:12
+0000
> +++ lib/lp/translations/model/potmsgset.py 2010-03-25 00:11:17
+0000
> @@ -888,6 +888,36 @@
> matching_message.sync()
> return matching_message
>
> + def submitSuggestion(self, pofile, submitter, new_translations):
> + """See `IPOTMsgSet`."""
> + if self.is_translation_credit:
> + # We don't support suggestions on credits messages.
> + return None
> +
> + potranslations = self._findPOTranslations(new_translations)
> +
> + existing_message = self._findTranslationMessage(
> + pofile, potranslations, pofile.plural_forms)
> + if existing_message is not None:
> + return existing_message

In the end you still included this logic as part of submitSuggestion? I
guess we can move it out later if it turns out that's what we want.

> === modified file 'lib/lp/translations/tests/test_potmsgset.py'
> @@ -1267,5 +1271,274 @@
> self.assertEqual(credits_type,
credits.translation_credits_type)
>
>
> +class KarmaListener:

Any reason not to put this inside lib/lp/registry/tests/karma.py?

> +class TestPOTMsgSet_submitSuggestion(TestCaseWithFactory):
> + """Test `POTMsgSet.submitSuggestion`."""
> +
> + layer = ZopelessDatabaseLayer
> +
> + def _makePOFileAndPOTMsgSet(self, msgid=None, with_plural=False):

In general, if you find that you need such a method, we might probably
need it in other cases as well. So, why not put it into the factory?

> + """Set up a `POFile` with `POTMsgSet`."""
> + pofile = self.factory.makePOFile('nl')
> + if msgid is None:
> + singular_text = self.factory.getUniqueString()
> + else:
> + singular_text = msgid
> + if with_plural:
> + plural_text = self.factory.getUniqueString()
> + else:
> + plural_text = None
> + potmsgset = self.factory.makePOTMsgSet(
> + pofile.potemplate, singular=singular_text,
plural=plural_text)
> +

Some extra whitespace here.

> + return pofile, potmsgset
> +
> + def _suggest(self, pofile, potmsgset, translations):
> + """Convenience shorthand for submitSuggestion."""
> + return potmsgset.submitSuggestion(
> + pofile, pofile.potemplate.owner, translations)

I believe this reduced the usefulness of these tests as a documentation
for submitSuggestion() method. And yet, you have added no documentation
whatsoever.

> + def test_plural_forms(self):
> + pofile, potmsgset =
self._makePOFileAndPOTMsgSet(with_plural=True)
> + translations = {
> + 0: self.factory.getUniqueString(),
> + 1: self.factory.getUniqueString(),
> + }
> +
> + suggestion = self._suggest(pofile, potmsgset, translations)
> + for form, translation in translations.iteritems():
> + self.assertEqual(
> + translation,
> + getattr(suggestion, 'msgstr%d' % form).translation)
> + for form in xrange(2, TranslationConstants.MAX_PLURAL_FORMS):
> + self.assertIs(None, getattr(suggestion, 'msgstr%d' %
form))

How does using these for loops improve readability of the test? FWIW,
what
does it bring other than making it harder to read the test? :)

At least the first for loop should be directly spelled-out so it's clear
what is tested.

> + def test_same_as_shared(self):
> + # A suggestion identical to a shared current translation is a
> + # repeated suggestion.
> + pofile, potmsgset = self._makePOFileAndPOTMsgSet()
> + translation = {0: self.factory.getUniqueString()}
> + shared_message = potmsgset.updateTranslation(
> + pofile, pofile.potemplate.owner, translation, True,
> + datetime.now(pytz.UTC), force_shared=True)

Any reason you are not using factory.makeSharedTranslationMessage here?
It will be of great help once we start getting rid of
updateTranslation()
calls (i.e. we'll only have to change behaviour in the factory).

> + def test_same_as_diverged(self):
> + # A suggestion identical to a diverged current translation
for
> + # the same template is a repeated suggestion.
> + pofile, potmsgset = self._makePOFileAndPOTMsgSet()
> + translation = {0: self.factory.getUniqueString()}
> + diverged_message = potmsgset.updateTranslation(
> + pofile, pofile.potemplate.owner, translation, True,
> + datetime.now(pytz.UTC), force_diverged=True)

Same as above, though use factory.makeTranslationMessage.

> + def test_same_as_diverged_elsewhere(self):

> + def test_same_as_hidden_shared(self):

Replace updateTranslation usage, please.

> + def test_credits_message(self):
> + # Suggestions for translation-credits messages are ignored.
> + pofile, potmsgset = self._makePOFileAndPOTMsgSet(
> + msgid='translator-credits')
> + translation = {0: self.factory.getUniqueString()}

It'd be nice to demonstrate that this message is_translation_credit.

 review needs-fixing code

Cheers,
Danilo