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

Revision history for this message
Robert Collins (lifeless) wrote :

129 """Validate the person is a real person with no other restrictions."""
130 +
131 def validate(person):

That new vertical whitespace is non-PEP8 - this is a curried function, so its appropriate to have it adjacent and not break the eye's flow reading past it - please don't do that. If your editor is telling you to do it, give it a good spanking.

You also have some list reflows that make things less readable - our style guide encourages readability over sheer compactness; I'm on the fence myself, but wanted you to be aware that you have discretion there - what the code had was fine, with the lines and indentation clearly grouping the query clauses.

The implementation of a 2-query look up looks pretty nice, I'm glad it turned out well. I can't really judge the ui result trivially, perhaps a screenshot of a complexish situation?

I haven't done a full code review, I think you should carry on with Maris or whomever is reviewer-de-jour.

« Back to merge proposal