Code review comment for lp:~thumper/launchpad/revision-karma-fix

Generally, I like the changes.

> === modified file 'lib/lp/code/model/revision.py'
> --- lib/lp/code/model/revision.py 2009-12-08 04:03:30 +0000
> +++ lib/lp/code/model/revision.py 2010-01-05 02:10:14 +0000
> @@ -86,10 +86,8 @@
> """See `IRevision`."""
> # If we know who the revision author is, give them karma.
> author = self.revision_author.person
> - if (author is not None and branch.product is not None):
> - # No karma for junk branches as we need a product to link
> - # against.
> - karma = author.assignKarma('revisionadded', branch.product)
> + if (author is not None):

These parens can go now.

> + karma = branch.target.assignKarma(author, 'revisionadded')
> # Backdate the karma to the time the revision was created. If the
> # revision_date on the revision is in future (for whatever weird
> # reason) we will use the date_created from the revision (which
> @@ -98,8 +96,12 @@
> # is lying), and a problem with the way the Launchpad code
> # currently does its karma degradation over time.
> if karma is not None:
> - karma.datecreated = min(self.revision_date, self.date_created)
> + removeSecurityProxy(karma).datecreated = min(
> + self.revision_date, self.date_created)
> self.karma_allocated = True
> + return karma
> + else:
> + return None
>
> def getBranch(self, allow_private=False, allow_junk=True):
> """See `IRevision`."""
> @@ -385,9 +387,6 @@
> from lp.registry.model.person import ValidPersonCache
>
> store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
> -
> - # XXX: Tim Penhey 2008-08-12, bug 244768
> - # Using Not(column == None) rather than column != None.
> return store.find(
> Revision,
> Revision.revision_author == RevisionAuthor.id,
> @@ -397,7 +396,8 @@
> Select(True,
> And(BranchRevision.revision == Revision.id,
> BranchRevision.branch == Branch.id,
> - Not(Branch.product == None)),
> + Or(Branch.product != None,
> + Branch.distroseries != None)),
> (Branch, BranchRevision))))
>
> @staticmethod

> === modified file 'lib/lp/code/scripts/revisionkarma.py'
> --- lib/lp/code/scripts/revisionkarma.py 2009-06-25 04:06:00 +0000
> +++ lib/lp/code/scripts/revisionkarma.py 2010-01-05 02:10:14 +0000
> @@ -39,8 +39,9 @@
> count = 0
> revision_set = getUtility(IRevisionSet)
> # Break into bits.
> - revisions = list(
> - revision_set.getRevisionsNeedingKarmaAllocated()[:100])
> + result_set = revision_set.getRevisionsNeedingKarmaAllocated()
> + self.logger.info("%d revisions to update", result_set.count())
> + revisions = list(result_set[:100])
> while len(revisions) > 0:
> for revision in revisions:
> # Find the appropriate branch, and allocate karma to it.

This looks a teensy bit strange to me - is it OK to call .count() on
the result then slice it? I guess it is, but I don't really like this
style:

 work = get_more_work()
 while work:
   do work
   work = get_more_work()

I'd prefer:

 while True:
   work = get_more_work()
   if not work:
     break
   do work

(which might annoy the structured programming purists but well...).

I'm not going to insist you change this in this branch though :-)

Overall, a nice change.

Cheers,
mwh