Code review comment for lp:~james-w/launchpad/get-requested-reviews

Revision history for this message
Eleanor Berger (intellectronica) wrote :

(16:27:14) thekorn: james_w, not that I have review powers, but there is a typo in xx-branchmergeproposal.txt: `Gettting` ;)
(16:28:44) intellectronica: james_w: we don't use line continuation in the launchpad codebase. instead use parentheses to group different constituents of an expression appearing on different lines
(16:29:45) intellectronica: i can see that the same problem appears in an existing lines. no idea why the author of that line decided to format it like that. if you feel like fixing that too, i won't complain :) (but don't feel obliged)
https://code.edge.launchpad.net/launchpad/+activereviews
(16:31:52) intellectronica: james_w: the tuple on line 103 of the diff is formatted a bit funny. why not just put it on one line?
(16:32:56) intellectronica: james_w: also, maybe rename 'collection' in the same function to something more meaningful, like 'user_visible_branches'?
(16:34:08) intellectronica: james_w: using title capitalization, isn't "Getting Merge Proposals a Person has been Asked to Review" the correct form?
jamalta james_w
(16:35:23) intellectronica: james_w: why the final comma on line 129?
(16:35:53) intellectronica: james_w: other than that it all looks good

review: Needs Fixing

« Back to merge proposal