Code review comment for lp:~jcsackett/launchpad/plus-participation-additional-fixes
Revision history for this message
| Edwin Grubbs (edwin-grubbs) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
0e1f616
(Get the code!)
Hi JC,
I just have a few comments more before it's ready to land.
-Edwin
> > 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.
When scanning code, a person often doesn't notice the very end of the line. Here is an extreme example to illustrate my point:
result = [
a in blist]
The user might think you are iterating over blist instead of alist. While your list comprehension isn't nearly as hazardous, I think putting the "if" at the start of the line will help readers parse it quickly.
> > 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 _getDirectMembe
> >> # never return None
> >> assert self.hasPartici
> >> @@ -1687,19 +1699,16 @@
> >> self.invited_
> >> orderBy=
> >>
> >> - # 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 myactivemembers
> >> + def team_membership
> >> """See `IPerson`."""
> >> - return TeamMembership.
> >> - TeamMembership.
> >> - Person.id = TeamMembership.team
> >> - """ % sqlvalues(self.id, [TeamMembership
> >> - TeamMembershipS
> >> - clauseTables=
> >> - orderBy=
> >> + Team = ClassAlias(Person, "Team")
> >> + store = Store.of(self)
> >> + return store.find(
> >> + And(TeamMembers
> >> + Team.id == TeamMembership.
> >> + TeamMembership.
> >> + [TeamMembership
> TeamMembershipS
> >
> >
> > 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(
> > 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 DecoratedResult
>
> Actually, you and I looked at teams_indirectl
> 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_
> (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.
I think it is good that you rewrote this as a storm query. My concern is that you are slowing down the query with a join that you aren't taking advantage of. You storm query will end up generating sql like this:
SELECT TeamMembership.*
FROM
TeamMembership,
Team
WHERE Team.id == TeamMembership.team
...
If you want storm to cache the team, you will have to rewrite it as
store.
which will give you the sql query:
SELECT TeamMembership.*, Team.*
FROM ...
If you don't want storm to fill its cache with the Team objects when this query runs, then you should remove the
Team.id == TeamMembership.
condition, since storm only adds it to the FROM clause automatically. It does not add it to the results, and it can't cache what it doesn't pull out of the database.
> > The style guide dictates arguments on multiple lines as either:
> > result = result.
> > 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?
The way you reformatted it looks good. Yes, it has to do with the indentation. The left side of each argument should line up. Here is a complicated example that is technically correct, although it should be avoided since it is so ugly.
doSomething(one,
four,
> 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_
> >> + graph.update(
> >
> >
> > There is no point adding all_user_
> > 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_
> > instead of (self, team) tuples, but that isn't necessary, since you have
> > added self.id to your query for all_direct_
> > need to create the variable all_user_
>
> 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.
It looks like you still have the double white spaces between "return" and the tuple.
>=== modified file 'lib/lp/
>--- lib/lp/
>+++ lib/lp/
>@@ -3098,11 +3098,10 @@
> [via_team.
>
> if membership is None:
>- # Membership is via an indirect team; sane defaults exist.
>-
>- # The user can only be a member of this team.
>+ # Membership is via an indirect team so sane defaults exist.
>+ # An indirect member cannot be an Owner or Admin of a team.
> role = 'Member'
>- # The user never joined, and can't have a join date.
>+ # The Person joined, and can't have a join date.
I think you mistakenly removed "never" from the sentence.
> datejoined = None
> else:
> # The member is a direct member; use the membership data.
>=== modified file 'lib/lp/
>--- lib/lp/
>+++ lib/lp/
>@@ -2070,25 +2060,33 @@
> """See `Iperson`."""
> # Get all of the teams this person participates in.
> teams = list(self.
>- all_user_
>
> # Get all of the memberships for any of the teams this person is
>- # a participant of.
>+ # a participant of. This must be ordered by date and id because
>+ # because the graph the results will create needs to contain
s/graph the/graph of the/
>+ # the oldest path information to be consistent with results from
>+ # IPerson.
> store = Store.of(self)
> all_direct_
> TeamMembership.