Code review comment for lp:~abentley/launchpad/fix-undiverging

Revision history for this message
Aaron Bentley (abentley) wrote :

> A note about the test: there's a factory.makeDivergedTranslationMessage for
> creating diverged messages.

There is, but we want this to be a *current* diverged message. makeCurrentTranslationMessage calls makeDivergedTranslationMessage when diverged=True.

> About the fix itself: approveSuggestion could be used very intensively, so for
> performance it may be worth shortcutting the getCurrentTranslation call (which
> queries the database) in cases where the message is diverged. In those cases,
> there's no chance of it being shadowed by another diverged message.

True. This change was obviously about correctness over performance.

Note that _setTranslation also calls getCurrentTranslation, so if I was optimising, I would first change _setTranslation to handle the case where the incumbent is the suggestion. Doing the check in approveSuggestion smells like Look-Before-You-Leap.

« Back to merge proposal