Hi Maris,

Thanks a lot for reviewing this over-sized branch!

У уто, 27. 07 2010. у 21:34 +0000, Māris Fogels пише:

> The code looks OK for this branch.  I like the object redesign so far.  The branch is actually too large for me to review well, but I tried anyway.  Here is what I saw:
> 
>  • I'm scared by code that passes around large argument lists like
>  ProductSeriesLanguage.setCounts().  Since  PSL already recieves a
>  reference to the pofile in it's constructor, is it possible to have
>  PSL pull the counts from the pofile itself internally?  Other options
>  are to create a new method PSL.setCountsFromPofile(), or create a new
>  object like PSLCounts(), or just pass in the calculated value:
>  PSL.setCounts(translated).

Not really: PSL will sometimes have only a single PO file (well,
interestingly, *most* of the time), but a generic case allows it to have
more than one.  And for all the stats that we show, we need all these
numbers.

The reason for it's existence is that you sometimes don't want to do a
single query for each of the languages you are fetching (eg.
https://translations.edge.launchpad.net/openobject-addons/trunk gets
aggregated stats from a bunch of stats for ~100 pofiles per language
with one very fast query), so we basically allow "outer" objects to
initialize PSL.  When we switch to a stats object itself, we'd be
setting each of them directly, so we won't be passing large arguments
list like this.

>  • interfaces/translatedlanguage.py should have a copyright of 2010

Fixed.
 
>  • The test in test_TwoTemplatesWithTranslations() could be split up
>  easily in a follow-up branch.  That one test method has at least two,
>  if not three, complete tests contained within it.

Actually, it also does a lot of assertions that are not needed for that
particular test. Like asserting behaviour that was already tested in
other tests.  I've removed those.

I've split it into three tests first, and then actually got rid of the
two that are already tested in test_translatedlanguage
(test_pofiles_two_templates and test_pofiles_two_templates_one_dummy).
The only one left is the one testing the single-POFile optimization
case.

So, thanks to you pointing this out, I've cleaned these tests so we
don't get them repeated between interfaces.  It is a separate branch
though (lp:~danilo/launchpad/psl-tests-cleanup).

>  • I found the test names in TestTranslatedLanguageMixin a bit vague. 
>  For example, "test_parent" looks something like
>  "test_psl_sets_translation_parent"

test_parent just tests that parent is correctly initialized for a new
object.  The naming is directly linked to the attribute being tested.  I
name unit tests like that (test_"attribute" or test_"method name", and
then add additional descriptors after that when needed).

> , and I can't tell from the tests
>  alone if the test_recalculateCounts_* methods verify the algorithm
>  correctly or not (maybe the Counts tests will be clearer when they are
>  rewritten to test the new Stats object instead of the Mixin).

These tests are not extensive, and they are going to be hard to figure
out as long as we used the messed-up RosettaStats interface (which
stores values we don't really care about, with weird names, and requires
everything we *do* care about to be calculated separately).

I also didn't change the interface much so the diff wouldn't grow too
much.  But, recalculateCounts() is probably going to turn into something
like initialize() instead.

>  • It would be nice to see tests for POFilesByPOTemplates in a
>  follow-up branch.  That way you don't have to test it indirectly with
>  expensive and verbose translation setup code in the Mixin tests, the
>  Mixin tests will shrink, and you may be able to skip the database
>  entirely with some fake objects.

Well, the focus of POFilesByPOTemplates is for it to be a tool to
basically allow "filling" ResultSet with non-DB objects while doing a
total of one DB query (without sacrificing slicing and/or batching).
So, I can't imagine it having any simpler set-up, and testing it with
fake objects is going to make tests useless (fwiw, if we were not aiming
for doing this, implementation of pofiles attribute would not use
POFilesByPOTemplates but would instead just construct a full list as a
combination of DB-based and in-memory objects, and then return that).

It would probably be even better to come up with a generic solution for
these kinds of things, but step at a time. :)

> Otherwise, I think this looks good.  With consideration of the points mentioned, r=mars.

Thanks,
Danilo