Code review comment for lp:~jtv/launchpad/bug-146855

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

 review approve

Approved conditionally on switching the database user in the test case
(see below)

Jeroen T. Vermeulen wrote:
>> Jeroen T. Vermeulen wrote:
>> Do you think AutoApproveProcess is still a good name, since it's also
>> responsible for other queue maintenance? What about ProcessImportQueue
>> or something?
>
> It wasn't a good name when I first saw it, to be honest. You're right, and
> I'm changing this. See below.

I like it.

>> It seems a bit odd to be wrapping the script in if __name__ = '__main__'
>> blocks. It can't be loaded as a module because of its name, and even if
>> it could, I don't see any value in it.
>
> It's standard practice; I believe some Python syntax checkers will fool
> themselves into thinking the script has an importable name. I know
> that at least one—don't remember which—does its work by importing even
> a main executable Python file as a module.

Fair enough.

>> Does it make sense to specify a timedelta here? If you always specify
>> your durations in days, it's less repetitive convert to a timedelta in
>> _cleanUpObsoleteEntries
>
> I thought it was nice and explicit. Storing number-of-days struck me as
> unnecessarily C-like; timedeltas let me store a nondescript "time
> interval" that you can just add to or subtract from dates. And who
> knows, maybe someday we'll want other kinds of intervals.

There often is a tension between being explicit and being succinct. I'm
fine either way.

>> If you're specifying a timedelta, shouldn't the comment say "Length of
>> time" instead of "Number of days"? Is FAILED really a terminal status?
>> It seems like it's possibly not terminal, and that's why you're
>> delaying 30 days.
>
> Absolutely. I should have updated that part of the comment as well now.

Hmm. I think you missed the "Is FAILED really a terminal status?"
question. Not that fixing a comment is worth delaying the branch.

>> I find the way the deletion_criteria is constructed a bit surprising.
>> What do you think about building up a list of clauses first, and then
>> ORing them? For example:

> I considered that and felt that it was a toss-up. But I'm used my ways
> of doing things, and now that you mention it, your way is clearer.
> Changed.

Cool.

>>> === added file 'lib/lp/translations/scripts/import_approval.py'
>> Since this does more than approving imports, is import_approval a good
>> name? Maybe queue_gardener?
>
> I renamed it import_queue_gardener.py, and the class ImportQueueGardener.

Great.

> While I was at it I also gave the script its own database user, which
> requires a lot fewer database privileges than the original one.

That change also looks good, but I'd like to see the database user being
set to the new value in the unit tests. IME, it's very easy to miss a
required DB permission unless you test with the script user.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkq32UUACgkQ0F+nu1YWqI39DQCeMlPGurWQ4CxwplR0CaKKBVJA
SgAAn3+54mFu6LLI2TXnVixy0LKGG9/O
=6+zS
-----END PGP SIGNATURE-----