Code review comment for lp:~jtv/launchpad/bug-403992

Hi Brad! Thanks for the review. This branch has been lying around for
a while, waiting for someone to review it. Unlike its cherry-pickable
sibling it's not urgent, but I'm glad to have it out of the way.

> I'm curious also about your/Danilo's choice to re-open a bug that was
> fix-committed to make these clean-up changes. Seems like an odd way
> to handle it, but not really any of my concern here.

I wouldn't say that was our "choice." What happened was that I prepared
two branches for the same problem: one immediate, absolutely minimal fix
for cherry-picking, and this conventional full fix as a superset of the
former. The cherry-picking branch has landed, and the bug went through
a brief tentative phase of being "fix committed" because of that. But
it all happened well after this branch here went up for review.

> > === modified file 'lib/lp/translations/scripts/message_sharing_migration.py'
> > --- lib/lp/translations/scripts/message_sharing_migration.py 2009-07-19
> 04:41:14 +0000
> > +++ lib/lp/translations/scripts/message_sharing_migration.py 2009-08-05
> 11:44:49 +0000
>
> Would you mind fixing the __all__ line to make it multi-line (is that our
> convention when there is just one entry?) or remove the extra spaces.

Don't mind at all. Done.

> > @@ -146,6 +147,8 @@
> >
> > class MessageSharingMerge(LaunchpadScript):
> >
> > + templateset = None
>
> How about 'template_set' as it follows our coding standard?

In English you never really know two words have become one until one of
them changes, like the "sheep" in "shepherd."

I think I chose "templateset" to distinguish the utility from a mere set
of templates. Then again, naming variables after their type should not
be taken this far anyway.

> > @@ -361,7 +370,13 @@
> > potmsgset (because we start out with one) and potemplate (because
> > that's sorted out in the nested dicts).
> > """
> > - return (tm.language, tm.variant) + tuple(tm.all_msgstrs)
> > + tm = removeSecurityProxy(tm)
> > + msgstr_ids = [
> > + getattr(tm, 'msgstr%dID' % form)
> > + for form in xrange(TranslationConstants.MAX_PLURAL_FORMS)
> > + ]
>
> The ] should be dedented by 4.

Ah, my editor is showing a mind of its own. Can't have that.

Hi ho, hi ho, to PQM I go!

Jeroen