Code review comment for lp:~julian-edwards/launchpad/ppa-expire-sources

Revision history for this message
Julian Edwards (julian-edwards) wrote :

On Tuesday 19 January 2010 19:30:24 Brad Crittenden wrote:
> Review: Approve code
> Hi Julian,
>
> The branch looks good. You mentioned renaming the file but deferred due to
> time constraints. Seems like it would take 10 seconds so you might as
> well do it now. expire_ppa_resources.py ?

I would have done that if it were not for the fact that the losas also need to
change their crontabs, which makes it a >10 second job :) But still, we'll
take the hit and do it. Thanks for prodding me.

> As far as refactoring the SQL it looks like you could use common aliases
> and then many of your comparisons would be the same, leaving you to just
> customize the WHERE clauses. Would that help?

I tried. It was really unreadable and the refactoring didn't help reduce the
size of the code that much.

> Or, if it was Storm-i-fied
> you may find it easier to refactor. Just two lame suggestions.

I'd love to do that but it's quite hard, STORM doesn't have the EXCEPT
command.

> Otherwise your change looks very well done. Thanks.

Thanks, I'll land this soon!

« Back to merge proposal