Code review comment for lp:~rvb/launchpad/branches-timeout-bug-827935

Hi Raphaël,

Nice branch! I've got a few comments but they're all cosmetic really. Otherwise, r=me.

[1]

62 +
63 + if self.simplified_branch_menu:
64 + return (
65 + self.registered_branch_not_empty or
66 + self.owned_branch_not_empty or
67 + self.subscribed_branch_not_empty or
68 + self.active_review_not_empty
69 + )
70 + else:
71 + return (self.owned_branch_count or
72 + self.registered_branch_count or
73 + self.subscribed_branch_count or
74 + self.active_review_count)

Slightly incosistent formatting here: The form you've used from
64-69 (newline after the opening parenthesis) is the correct one, though
the closing paren should go on the last line as you've done on line 74.

[2]

89 + @cachedproperty
90 + def owned_branch_not_empty(self):
91 + """False if the number of branches owned by self.person is zero."""
92 + return not self._getCountCollection().ownedBy(self.person).is_empty()

I get the feeling this should be named "owned_branches_not_empty" since
you're talking about a collection but the property name suggests you're
talking about a single branch.

[3]

94 + @cachedproperty
95 + def subscribed_branch_not_empty(self):

... And by the same token this should be subscribed_branches_not_empty.

[4]

103 + @cachedproperty
104 + def active_review_not_empty(self):

active_reviews_not_empty

[5]

224 + registered_branches_matcher = soupmatchers.HTMLContains(
225 + soupmatchers.Tag(
226 + 'Registered link', 'a', text='Registered branches',
227 + attrs={'href': 'http://launchpad.dev/~barney'
228 + '/+registeredbranches'}))
229 +

By convention, if you're going to declare a class attribute, you should
do it at the top of the class, since otherwise the reader (i.e. your
humble bearded reviewer) gets a bit confused :).