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

Revision history for this message
Raphaƫl Badin (rvb) wrote :

> [1]
>
[...]
> 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.

Right, I've only written the first part, but I should have harmonized it with the copied code indeed.
Fixed.

> [2]
> [3]
> [4]
> [5]

Right. Done.

> 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 :).

Okay. I thought it might be a good idea to declare it near where it's used but I suppose it's much better to have a sane default for these.

Thanks for the review Graham!

« Back to merge proposal