Code review comment for lp:~jcsackett/launchpad/plus-participation-additional-fixes

More detailed reply with updated code...

> I am impressed with all that you have accomplished in this branch. Most
> of my comments are related to formatting, which is to be expected with
> new employees. I felt your pain.

Actually, once I ran through some experiments in a terminal, it wasn't *that* painful. But thanks. :-)

> You really need to run "make lint" against the branch, especially
> test_person.py.

I thought I had, but I think I ran make lint before committing some files, and so only got the issues with those files.

> The style guide doesn't explicitely mention this, but I think the
> formatting choices should be similar to a function call. Each line of
> the list comprehension should line up so they are easier to read:
> Either:
> result = [foo
> bar
> baz]
> Or:
> result = [
> foo
> bar
> baz]
>
> For actual lists, the ending square bracket has to be on a line by
> itself. There isn't an exact rule for list comprehensions, but
> pocketlint will complain if you put the square bracket on a line by
> itself without a comma on the previous line, and of course, that only
> works for lists.

I've broken up the list comprehension as

indirect_teams = [
            team for team in paths.keys() if
            team not in direct_teams]

It seems clean to break the conditional filter onto its own line.

> I don't see the limit being used anywhere? Is that feature still needed?
>
> If it's needed the docstring should describe what limit is. Look in other
> docstrings, which use the convention:
> :param PARAMNAME: DESCRIPTION

The limit isn't being used anywhere, but I had left it in b/c I could see use of this method
being a problem for someone else down the road. However, I can see it's presence now
is just confusing, and it's a sufficiently obvious solution that if it's actually needed down
the road it can be redone then.

>
>
>> # This is our guarantee that _getDirectMemberIParticipateIn() will
>> # never return None
>> assert self.hasParticipationEntryFor(team), (
>> @@ -1687,19 +1699,16 @@
>> self.invited_members,
>> orderBy=self._sortingColumnsForSetOperations)
>>
>> - # XXX: kiko 2005-10-07:
>> - # myactivememberships should be renamed to team_memberships and be
>> - # described as the set of memberships for the object's teams.
>> @property
>> - def myactivememberships(self):
>> + def team_memberships(self):
>> """See `IPerson`."""
>> - return TeamMembership.select("""
>> - TeamMembership.person = %s AND status in %s AND
>> - Person.id = TeamMembership.team
>> - """ % sqlvalues(self.id, [TeamMembershipStatus.APPROVED,
>> - TeamMembershipStatus.ADMIN]),
>> - clauseTables=['Person'],
>> - orderBy=Person.sortingColumns)
>> + Team = ClassAlias(Person, "Team")
>> + store = Store.of(self)
>> + return store.find(TeamMembership,
>> + And(TeamMembership.personID == self.id,
>> + Team.id == TeamMembership.teamID,
>> + TeamMembership.status.is_in(
>> + [TeamMembershipStatus.APPROVED, TeamMembershipStatus.ADMIN])))
>
>
> I know I looked at this code before with you, but I don't
> understand why you are joining with the Team here, since you are not
> actually pulling the Team info out with the TeamMembership. If that is
> what you are trying to do, then you need something like this:
> result = store.find((TeamMembership, Team),
> And(...))
> If you don't want team_memberships to return tuples with both items, you
> do this:
> def decorate(row):
> membership, team = row
> return membership
> return DecoratedResultSet(result, decorate)

Actually, you and I looked at teams_indirectly_participated_in; this is just another query used in the process
that sinzui and I thought would benefit from being rewritten as a storm query; I mostly just ported the sql
directly from the original SQLObject query. Since it was used in more places than just active_participations
(and now isn't used there at all) I didn't want to alter it more.

Given that the area we were trying to improve no longer uses the method, I suppose an argument could
be made to just remove the changes, but it seems like the changes are largely beneficial anyway.

> The style guide dictates arguments on multiple lines as either:
> result = result.order_by(one,
> two)
> Or:
> result = result.order_by(
> one, two)
> Or:
> result = result.order_by(
> one,
> two)

I confess to being a touch confused by this note: near as I can tell the current line follows the first convention; is
the issue the degree of indentation?

In any event I have reformatted this to follow the last example, as it seems the easiest to read in the current code.

>> + graph = dict(all_direct_memberships)
>> + graph.update(all_user_memberships)
>
>
> There is no point adding all_user_memberships to the graph, since
> you only look up an item in the graph if it is set to the step, and as
> soon as the step is set to self, you break out of the loop. Perhaps, you
> had intended that all_user_memberships contain (team, self) tuples
> instead of (self, team) tuples, but that isn't necessary, since you have
> added self.id to your query for all_direct_memberships. You don't even
> need to create the variable all_user_memberships.

This one tripped me up; at an earlier point in my creation of this method the update was absolutely necessary; without it
the results were reliably wrong. I guess I hadn't updated my understanding as I finished mapping out the steps in the
traversal.

>> + return paths, user_memberships
>
>
> The extra space looks odd. If you want you can use parens to make the
> separation more prominent.

Agreed; additionally, parens make it more explicit that the method is returning a tuple.

I'm assuming the return statement is what you were referring to; correct me if I erred.

Thanks again for all the notes, Edwin.