Code review comment for lp:~mvo/launchpad/support-timeframe-fix-660433

Revision history for this message
Michael Vogt (mvo) wrote :

Thanks for the review Jeroen and thanks for the meta-review from Martin as well!

I update the code to adress a lot of the points, but there is more to be done. I'm happy
about the detailed review as it means I can just go over it point by point. The result
is that the code is now better and more readable. Martin is right, I don't usually do LP
development and I'm not familiar with the processes. I just want to fix this bug as it
affects the distro (badly) :)

I do not nessecarily agree with all of the points in the review (e.g. I think short variable
like "d" are fine if they are just used in a two lines or a short function). But that is fine, its more important that its consistent with the rest of the codebase.

The only "grief" I have is the testing template. To test a small part of LP like this
that does not need the database or other fancy part using the template drags in a huge
set of python dependencies, including psycopg2, storm, transactions, windmill
(that appears to be not packaged in ubuntu maverick) and more. It would be great to be
able to use the test template without all this if its not actually needed for the test
at hand.

« Back to merge proposal