Wow, big branch.  I know it wasn't easy.  Lots of goodness here, including lint cleanups all over the place.  But you also inherited some uglier things, and it seems to me that your "make lint" run was incomplete somehow.  I hope we can make some more headway with that, but in the interest of getting the branch in it's not as important as cleaning up the new bits.

We discussed some of these on IRC, but most we didn't.


=== lib/lp/testing/factory.py ===

Might as well make the "translations" parameter to _makeTranslationsDict default to None.

The docstring for _makeTranslationsDict goes "Make sure translations are in the right format."  That's a bit ambiguous as to what happens when the translations are not in the right format.  Consider saying something like "Convert translations" (and adding a note saying that it returns the original translations if they are already in the desired format).

In the same docstring, don't lead with "the right format"--name the format!  An example like '{0: "foo"}' may say it most effectively.

Naming all types of sequence that might make up a non-dict translations list is probably a losing game: is it really necessary to accept tuples?  What about ResultSets?  If it's really needed, consider rearranging the conditions: "if translations is None / elif isinstance(translations, dict) / else" (where the "else" part returns dict(enumerate(translations))).

In makeSuggestion: it's no longer necessary to setSequence on a POTMsgSet you get from makePOTMsgSet.  Just pass a sequence number to makePOTMsgSet.

In makeSuggestion, this line lacks spaces around the assignment operator:

        translations=self._makeTranslationsDict(translations)

And why does it set naked_translation_message.date_created to date_updated?  I'd expect date_updated to set date_updated, not date_created.


== lib/lp/translations/browser/tests/test_pofile_view.py ==

Thanks for cleaning up the lint!  While you're at it, could you chuck the entire test_suite function?  We no longer need those, so they're just cruft now.


== lib/lp/translations/browser/translationmessage.py ==

More lint cleanups.  Good show.


== lib/lp/translations/tests/test_pofile.py ==

There's a lot of code here that you clearly only moved, so you're probably more a victim than a perpetrator.  But still I hope you can fix some of this, if it doesn't mean too much extra work.

In setUp, this comment:

# POTemplate is 'shared' if it has the same name ('messages').

...is rather confusing.  I'd rather say that two POTemplates share if they have the same name.  And separately, that these particular two templates both have "messages" for a name.

In the names devel_sr_pofile and stable_sr_pofile, unless the "sr" part is actually relevant to the tests, I'd leave it out of the names.

This comment:

# Create a single POTMsgSet that is used across all tests,
# and add it to only one of the POTemplates.

...commits the standard mistake of repeating what the code does.  That's useless.  The only interesting part is that the potmsgset is only added to one of the templates...  *Why* is that?

And right below that, once again: no need to self.potmsgset.setSequence.  Just pass the sequence number to makePOTMsgSet.

In test_getPOTMsgSetWithNewSuggestions_Shared, is Shared supposed to be capitalized?

This test method, by the way, tests 4 separate situations.  That's a lot for one test, and risks sliding back into bad old doctest practice.  Could you break it down a bit?

In line 203 of the diff, this line again lacks spaces around an operator and I don't see how "make lint" could have accepted it:

        date_created = datetime.now(pytz.UTC)-timedelta(5)

In line 207, this line has space only on side of an operator:

        translation.is_current_ubuntu= True

This assertion in line 212 has the expected and actual values switched around:

        self.assertEquals(found_translations, [])

The same goes for line 219, where we might as well use assertFalse:

        self.assertEquals(suggestion.is_current_ubuntu, False)

...And so on for all other assertions I see in the test.

Then, test_getPOTMsgSetWithNewSuggestions_Diverged.  Again, why the capitalized word at the end?  And again, could you break this 4-stage scenario down into smaller tests?

There's another pair of missing spaces in line 258, but I'm tired of playing human linter.  Please re-run "make lint" and fix!

The comment in line 264 is missing an article before "shared current translation."

The comment in line 281 (again missing an article) talks about "Shared suggestion."  I think what is meant is just "The suggestion."

The comment in line 286 says "When a diverged translation is done after the shared suggestion."  Can you find a more descriptive verb than "done"?

In line 310, this comment surprises me:

# Setting a suggestion as current makes it have no unreviewed
# suggestions.

As far as I know, that only happens if the review date is also updated.  (The creation or last-update date may be used as a fallback if the review date is not set; I don't recall off the top of my head).  In that case, the new current translation is presumably newer than all suggestions and so leaves no new suggestions.  But the test code there only sets the flag.  I think this passes more or less by accident; what if there's another suggestion that's newer than the suggestion that's being made current?

In line 519 of the diff, you pass translations=[None] to makeSuggestion.  Is that None actually needed?  Or would a simple [] be enough?  The same goes for line 675, in lib/lp/translations/tests/test_potmsgset.py.


== lib/lp/translations/utilities/sanitize.py ==

In line 874 of the diff I see code that's also in the new _makeTranslationsDict.  Is there no good way of sharing it between the usees?

    if isinstance(translations, (list, tuple)):
        translations = dict(enumerate(translations))


== lib/lp/translations/utilities/tests/test_sanitize.py ==

Love test_sanitize_translations_not_in_dict.  Very easy to read.