launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Thanks for the review!

On Thu 14 Jun 2012 14:45:23 EST, William Grant wrote:
> Review: Approve code
>
> As discussed on IRC, this branch makes the jobs completely unsuitable for product. Most notably, the jobs in a lot of cases end up verifying the subscription consistency of hundreds of thousands or millions of unrelated bugs. However, it's a good intermediate step, since the jobs remain disabled on production.
>

I'll add it pillar filtering prior to landing.

> Since it doesn't make a huge amount of sense to review the functionality, there's just a couple of style comments. In addition to the below, I also propose http://pastebin.ubuntu.com/1040252/ as a bit of a cleanup around the admin special case.

Looks good I think.

>
> 185	bug_ids = [
> 186	- bug.id for bug in bugs
> 187	+ bug.id for bug in bugs or []
> 188	+ ]
> 189	+ information_types = [
> 190	+ info_type.value for info_type in information_types or []
> 191	]
>
> Can't those list comprehensions be on one line each?
>

The first one can. The second won't fit. To me it looked better if they 
were both done the same way. But I've changed it.

> 202	+ @property
> 203	+ def information_types(self):
> 204	+ return [
> 205	+ enumerated_type_registry[InformationType.name].items[value]
> 206	+ for value in self.metadata['information_types']]
>
> Why does this use the registry? You are looking the enum up by the name you got from the enum :)
>

I think you misread it :-) It's pulling serialised values (the names) 
from the stored json and looking them up in an items list associated 
with the InformationType.name attribute.

> 495	+# def test_revokeAccessGrantsBranches(self):
>
> I'm not sure a commented test is useful, particularly when we have the work planned and the test won't ever work as written.

The test worked fine in the previous branch. I had left it in as a 
reminder so it could be adapted when the next job was added but will 
remove it.


-- 
https://code.launchpad.net/~wallyworld/launchpad/unshare-team-members-subscriptions2-1009358/+merge/110215
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.