Am 16.08.2010 16:37, schrieb Jeroen T. Vermeulen:
> 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))).

I fixed all this as per our IRC discussion.

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

Ah yes, I copied and pasted makeSuggestion from makeTranslationMessage. I
added fixed it there, too. Funny enough, it was me who introduced the
parameter to the factory method in the first place. ;)

> 
> In makeSuggestion, this line lacks spaces around the assignment operator:
> 
>         translations=self._makeTranslationsDict(translations)

Yes, I had it directly in the function call at first. Thanks.

> 
> 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.
> 

Again, copied from makeTranslationMessage where it is a bit more approriate
although it sets "date_created" and "date_reviewed" there. I renamed the
parameter in makeSuggestion to date_created and update all call sites.


> 
> == 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.

Ah yes, right. I am always happy to remove cruft! ;-)

> 
> 
> == lib/lp/translations/browser/translationmessage.py ==
> 
> More lint cleanups.  Good show.

Thank you for the applause! ;)

> 
> 
> == 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.

Well, let's see .. ;-)

> 
> 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.

Easy. Changed. In both test cases ;-)

> 
> 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.

Find & Replace but it makes the branch a whole lot bigger...

> 
> 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?

I find the "why" hard to explain. "To demonstrate message sharing."? I did not
try to explain, they will have to read the tests ... ;-)

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

Yes, yes. Fixed.

> 
> In test_getPOTMsgSetWithNewSuggestions_Shared, is Shared supposed to be capitalized?

Probably not.

> 
> 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?

No. Sorry. That would have to be an extra tech debt branch. I guess the whole
file could use some updating in this respect.

> 
> 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)

True, it is really strange that it missed that.

> 
> In line 207, this line has space only on side of an operator:
> 
>         translation.is_current_ubuntu= True

Strange, too. Well spotted.

> 
> 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.

Yes, I did fix that in some places but stopped because it would have gotten
out of hand. I am not going to start again now, sorry.

> 
> 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?

Changed. And "No".

> 
> 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!

I'd say the linter is broken. There was nothing relevant disabled, either.

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

My guess: Serbian or Spanish speaker, which ever language does not use
indistinct articles. I don't always fix these even if I see them because it
gives our code this international, personal touch. Does it not? ;-)

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

Or "A 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"?

added.

> 
> 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?

Didn't the old validators do all that? I agree though, that this most likely
passes by accident now and I am pretty sure there may be more tests that do.
This branch was meant as a mechanical replacement, though, so I think trying
to check and fix all tests is beyond the scope.

> 
> 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.

Well, let's try .... Yes, you are right! Thanks!

> 
> 
> == 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))

Not anymore now. ;-) Also, I think the overhead would not be justified and
dict(enumerate()) is a simple enough (only uses builtin functions) to stand on
its own.

> 
> 
> == lib/lp/translations/utilities/tests/test_sanitize.py ==
> 
> Love test_sanitize_translations_not_in_dict.  Very easy to read.
> 

I am happy to bring you joy!

Thank you for your thorough review. I am sorry that I flat our refused some
requests but I really want to get this branch out the door now ...

Cheers,
Henning