Code review comment for lp:~henninge/launchpad/recife-bug-611674-convert-imports

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 "recife" work?

https://bugs.edge.launchpad.net/rosetta/+bug/655636

Thank you very much!

Henning