Code review comment for lp:~adiroiban/launchpad/bug-497438

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

This looks pretty good. I asked for a different name for utc_browser because people looking at the code will assume UTC stands for Universal Coordinated Time, and it was renamed "dtc_browser", which I think is fine.

It seems that we must do extra work to ensure that Persons used in doctests have pre-determined credentials so that we can hardcode them when calling setupBrowser. It would be nice if we had an alternative to setupBrowser that accepted arbitrary Persons, but it's not required for this patch.

By assigning to ubuntu.translationgroup, setupUTCBrowser has side effects that mean repeated calls will invalidate previous values. I was willing to accept this, since repeated calls should not be necessary, but Adi decided this should be fixed before landing.

review: Needs Fixing (code)

« Back to merge proposal