Merge lp:~henninge/launchpad/recife-bug-611674-convert-imports into lp:~launchpad/launchpad/recife

Hi Henning,

Thanks for tackling this. You've done a part of another card with this
job, so good job there :)

I feel it still needs a few improvements. See below.

  review needs-fixing

> A large part of the diff is made up of the new property
> "FileImporter.share_with_other_side" and its tests. The property is a
> boolean that reflects the sharing policy for translations from the
> file that is being imported. The policy depends on where the
> translations are coming from and what they are being imported into and
> on the permissions of the uploader. This was isolated to be able to
> change this policy later. (And I admit it could have warranted an
> extra branch.)

Interesting approach. We'll need to extract this even further, but
we've already got a separate card for that. You've just made that job
simpler (though, it's not going to be identical, since we'll have to
support the specifics of the web UI as well).

> === modified file 'lib/lp/translations/doc/poimport-script.txt'
> --- lib/lp/translations/doc/poimport-script.txt 2010-09-21 16:19:47
> +0000
> +++ lib/lp/translations/doc/poimport-script.txt 2010-09-30 08:53:45 +0000
> @@ -282,9 +282,9 @@
>
> >>> from lp.services.worlddata.interfaces.language import ILanguageSet
> >>> esperanto = getUtility(ILanguageSet).getLanguageByCode('eo')
> - >>> foos = potemplate['Foo %s'].getLocalTranslationMessages(
> + >>> suggestions = potemplate['Foo %
> s'].getLocalTranslationMessages(
> ... potemplate, esperanto)
> - >>> sorted([foo.msgstr0.translation for foo in foos])
> + >>> sorted([sugg.msgstr0.translation for sugg in suggestions])
> [u'Bar', u'Bars']

I don't really like "sugg" much better than "foo" (though, "suggestions"
instead of "foos" IS MUCH better :). How about "suggestion"?

> === modified file 'lib/lp/translations/utilities/sanitize.py'
> --- lib/lp/translations/utilities/sanitize.py 2010-08-16 09:01:35 +0000
> +++ lib/lp/translations/utilities/sanitize.py 2010-09-30 08:53:45 +0000
> @@ -181,6 +181,8 @@
>
> # Expected plural forms should all exist and empty translations should
> # be normalized to None.
> + if pluralforms is None:
> + pluralforms = 2
> for pluralform in range(pluralforms):
> if pluralform not in sanitized_translations:
> sanitized_translations[pluralform] = None

What's the reasoning for this? Why not default to 1 instead (i.e.
assume it's a message with no plurals at all)?

> === modified file 'lib/lp/translations/utilities/tests/test_sanitize.py'
> --- lib/lp/translations/utilities/tests/test_sanitize.py 2010-08-23 08:41:03 +0000
> +++ lib/lp/translations/utilities/tests/test_sanitize.py 2010-09-30 08:53:45 +0000
> @@ -257,6 +257,20 @@
> expected_sanitized,
> sanitize_translations_from_webui(self.english, translations, 3))
>
> + def test_sanitize_translations_pluralforms_none(self):
> + # Some languages don't provide a plural form, so 2 is assumed.
> + translations = {
> + 0: u'Plural form 0 ',
> + }
> + expected_sanitized = {
> + 0: u'Plural form 0',
> + ...

> > === modified file 'lib/lp/translations/doc/poimport-script.txt'
> > --- lib/lp/translations/doc/poimport-script.txt 2010-09-21 16:19:47
> > +0000
> > +++ lib/lp/translations/doc/poimport-script.txt 2010-09-30 08:53:45 +0000
> > @@ -282,9 +282,9 @@
> >
> > >>> from lp.services.worlddata.interfaces.language import ILanguageSet
> > >>> esperanto = getUtility(ILanguageSet).getLanguageByCode('eo')
> > - >>> foos = potemplate['Foo %s'].getLocalTranslationMessages(
> > + >>> suggestions = potemplate['Foo %
> > s'].getLocalTranslationMessages(
> > ... potemplate, esperanto)
> > - >>> sorted([foo.msgstr0.translation for foo in foos])
> > + >>> sorted([sugg.msgstr0.translation for sugg in suggestions])
> > [u'Bar', u'Bars']
>
> I don't really like "sugg" much better than "foo" (though, "suggestions"
> instead of "foos" IS MUCH better :). How about "suggestion"?

Yeah, I didn't think it would fit. It does.

>
> > === modified file 'lib/lp/translations/utilities/sanitize.py'
> > --- lib/lp/translations/utilities/sanitize.py 2010-08-16 09:01:35 +0000
> > +++ lib/lp/translations/utilities/sanitize.py 2010-09-30 08:53:45 +0000
> > @@ -181,6 +181,8 @@
> >
> > # Expected plural forms should all exist and empty translations should
> > # be normalized to None.
> > + if pluralforms is None:
> > + pluralforms = 2
> > for pluralform in range(pluralforms):
> > if pluralform not in sanitized_translations:
> > sanitized_translations[pluralform] = None
>
> What's the reasoning for this? Why not default to 1 instead (i.e.
> assume it's a message with no plurals at all)?

That's what Language does. I could remove it here and use language.guessed_pluralforms when calling the function.
Also, there is something else I was wondering about. The function will fill in the missing plural forms with None, regardless of whether there is a msgid_plural or not. I don't remember if that is intended behavior. It does no harm but seems like a waste.

>
> > === modified file
> 'lib/lp/translations/utilities/tests/test_file_importer.py'
> ...
> > +class FileImporterSharingTest(TestCaseWithFactory):
> > + """Class test for the sharing operation of the FileImporter base
> class."""
> > + layer = LaunchpadZopelessLayer
> > +
> > + UPSTREAM = 0
> > + UBUNTU = 1
> > + LANGCODE = 'eo'
>
> I know this is how we usually used to do stuff, but today we should be
> able to also make languages and not depend on existing ones in the
> sampledata. For instance, if you really want to keep using 'eo', I'd
> suggest you do factory.makeLanguage('eo@test').

No, I am not intend on this. Actually I defined it here so that it does not appear in the code itself. Changed it to use a constructed Language.

>
> > + POFILE = dedent("""\
> > + msgid ""
> > + msgstr ""
> > + "PO-Revision-Date: 2005-05-03 20:41+0100\\n"
> > + "Last-Translator: FULL NAME <EMAIL@ADDRESS>\\n"
> > + "Content-Type: text/plain; charset=UTF-8\\n"
> > + "X-Launchpad-Export-Date: 2009-05-14 08:54+0000\\n"
> > +
> > + msgid "Thank You"
> > + msgstr "Dankon"
> > + ...

Incremental diff.

У пет, 01. 10 2010. у 14:43 +0000, Henning Eggers пише:

> That's what Language does. I could remove it here and use
> language.guessed_pluralforms when calling the function. Also, there is
> something else I was wondering about. The function will fill in the
> missing plural forms with None, regardless of whether there is a
> msgid_plural or not. I don't remember if that is intended behavior. It
> does no harm but seems like a waste.

Let's keep it simple for now and not touch anything. It definitely
sounds a bit wasteful, but it's not a big deal.

> > I know this is how we usually used to do stuff, but today we should be
> > able to also make languages and not depend on existing ones in the
> > sampledata. For instance, if you really want to keep using 'eo', I'd
> > suggest you do factory.makeLanguage('eo@test').
>
> No, I am not intend on this. Actually I defined it here so that it does
> not appear in the code itself. Changed it to use a constructed
> Language.

Cool, thanks.

> > > + if side == self.UPSTREAM:
> > > + potemplate = self.upstream_template
> > > + else:
> > > + # Create a template in a source package and link the source
> > > + # package to the upstream series to enable sharing.
> > > + ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
> > > + distroseries =
> > self.factory.makeDistroSeries(distribution=ubuntu)
> > > + ubuntu.translation_focus = distroseries
> > > + sourcepackagename = self.factory.makeSourcePackageName()
> > > + potemplate = self.factory.makePOTemplate(
> > > + distroseries=distroseries,
> > > + sourcepackagename=sourcepackagename,
> > > + name=self.upstream_template.name)
> > > + self.factory.makeSourcePackagePublishingHistory(
> > > + sourcepackagename=sourcepackagename,
> > > + distroseries=distroseries)
> >
> > Why is this necessary? Is it not possible to reach a sourcepackage
> > without it?
>
> For some reason I cannot use sourcepackage.productseries if that has not been set.

Right, it's probably useful knowledge worth sharing across the
translations team at least. Knowing the background would be even better
because then we could reconstruct it in the future :)

> >
> > > + return entry
> > > +
> > > + def test_translator_persmissions(self):
> >
> > s/perSmissions/permissions/

You seem to have forgotten this bit.

> >
> > > + def test_templates_are_sharing(self):
> > > + # Sharing between upstream and Ubuntu was set up correctly.
> > > + entry = self._makeEntry(self.UBUNTU)
> > > + subset = getUtility(IPOTemplateSet).getSharingSubset(
> > > + distribution=entry.distroseries.distribution,
> > > + sourcepackagename=entry.sourcepackagename)
> > > + self.assertContentEqual(
> > > + [entry.potemplate, self.upstream_template],
> > > + list(subset.getSharingPOTemplates(entry.potemplate.name)))
> >
> > This looks a lot like a test for the code that's not really here,
> > doesn't it? :)
> >
> > Or, is it basicall...

Am 05.10.2010 13:34, schrieb Данило Шеган:
>>>> + self.factory.makeSourcePackagePublishingHistory(
>>>> + sourcepackagename=sourcepackagename,
>>>> + distroseries=distroseries)
>>>
>>> Why is this necessary? Is it not possible to reach a sourcepackage
>>> without it?
>>
>> For some reason I cannot use sourcepackage.productseries if that has not been set.
>
> Right, it's probably useful knowledge worth sharing across the
> translations team at least. Knowing the background would be even better
> because then we could reconstruct it in the future :)

Hm, should I put a card in the backlog to research that?

>
>>>
>>>> + return entry
>>>> +
>>>> + def test_translator_persmissions(self):
>>>
>>> s/perSmissions/permissions/
>
> You seem to have forgotten this bit.

I did, thanks. ;)

> It'd still be nice to name the test so it reflects this: eg.
> "test_makeImportEntry_templates_are_sharing".

Renamed.

>
>>> Cool stuff. It's amazing how tests can be so simple for something this
>>> intricate. :)
>>
>> I see the smiley but I am not sure if you are making fun of me or not ... ;-/
>
> No, it's a smiley of happiness. Also a credit to your "factor out stuff
> that is not being tested" (your _makeImportEntry method).

Thank you. :-)

>>>> + @cachedproperty
>>>> + def translations_are_msgids(self):
>>>> + """Are symbolic msgids being used and these are the real ones?"""
>>>
>>> This sentence is a bit confusing. If I had no idea about what this is
>>> about, I'd have even less idea about it. :)
>>
>> OK, I tried better. ;-)
>
> I think "text" is a non-countable noun, so plural "texts" sounds weird:

It can be both. In this case it is countable

>>> This is actually very interesting, and I am happy to see that you
>>> extracted it out. It actually points out that the naming for
>>> "from_upstream" is not really perfect: it should probably be
>>> "from_maintainer" or "from_trusted" or something.
>>
>> Yes, Jeroen and I had talked about that, too. I think I like from_maintainer most.
>
> Let's add a card for that then.

Done.

>>> Oh, I just read up on any() :) It seems you can drop the comparison to
>>> the empty list as well.
>>
>> Good catch! I was a bit worried about using "any" because it tests for
>> "False" but we are looking for empty strings and "None". Although
>> these are by definition "False", too, our coding standards always
>> forbade implicit bool checks like these.
>
> That was kind of my worry as well, but I guess there's no harm since the
> intent is clear enough.

I put it on the reviewer meeting agenda.

>
>>> With the new model, there will be no way to know if a translation
>>> credits message didn't validate, but is the actual one from upstream.
>>> Something to think about.
>>
>> Hm, we could still validate and mark the message accordingly, even if
>> it is used, right? I am not sure, though, if translators expect us to
>> catch bugs like that. Maybe we should discard c-format flags on
>> translation credits POTMsgSets altogether?
>
> Definitely worth thinking about some more. How about you file a bug
> about this to look into after all the "re...