[wiredfool]

A checklist can help. (he says, and then does a mental one because there isn't one nearby).

What I look for is:

1) What's the problem being solved? Does this look like a reasonable approach? Is the code pythonic (Obv: for python)?

2) What edge cases are there? Does this handle the important ones? Does it punt properly on the less important ones?

3) Look for a short list of bug classes that have come up in the project before that have lead to emergency patches. E.g. Decrefing, Checking mallocs, any exec sorts of things. (This is a clear application for a checklist)

4) Are there tests/documentation/other required fixtures and stuff?

5) Does the code generally match the style of the project?

1000) Code formating and whitespace and line wrapping and all that bikeshedding stuff.

Feel free to short circuit anywhere once it becomes clear that there's more work required.

[kripke]

1000 should really be handled by automated tools. Takes useless burden from the reviewer, and emotionally easier for both sides too.

[wiredfool]

It can be, especially at a company. (But then watch the bikeshed discussion on the tools. And PEP8 is a guideline, not a set of hard and fast rules. Beautiful is better than ugly and all that. ).

What I see as one of 5 maintainers of an open source project is that when a review comes back with a bunch of formatting comments, it's because the reviewer didn't step back and see the other parts. Raymond Hettinger has a talk where he discusses it, but it's the sort of feedback that can be given in almost any case and can obscure the more fundamental issues with the code.