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

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

= Bug 403992 =

This is a more complete fix for bug 403992 than the one I'm hoping to
cherrypick from lp:~jtv/launchpad/cp-bug-403992. You could see it as
all-part-of-the-service aftercare.

Besides the fix in that branch, this one adds some related improvements:

 * Make sure the database knows it's dirty after running the script in
   the doctest. It doesn't seem to change anything, but it's the right
   thing to do.

 * Cache some stuff in the script object, mostly so the identifiers get
   a bit shorter and the main code easier to read. A matter of taste,
   mostly.

 * When trying to diverge a shared message that can't be accommodated as
   shared on the representative potmsgset, try it on the most
   representative of the potemplates it participates in first, then the
   second-best, and so on. This is slightly more proper than sorting
   purely by id, although it probably doesn't matter too much.

 * Cull some now-dead methods and variables: current_tms, imported_tms,
   _findUsedMessages. And their unit tests, of course.

 * Make the unit tests' setUp pass test_args to the script object
   constructor. Otherwise the script would receive the arguments that
   the test driver (./bin/test) got on the command line. That _usually_
   isn't a problem, but when the test driver invokes itself recursively
   to run this test, it adds a --resume-layer argument which of course
   means nothing to the script object. This broke the unit tests in
   some combined test runs, but not in individual ones or more fortunate
   combinations.

To test,
{{{
./bin/test -vv -t message.sharing
}}}

To Q/A, run this on staging:
{{{
scripts/rosetta/message-sharing-merge -vvv -P -p elisa
}}}

No lint complaints.

Jeroen

« Back to merge proposal