Any tips on how to improve this?
Would a checklist help? Have a clear process on what to review first?
Any tips on how to improve this?
Would a checklist help? Have a clear process on what to review first?
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.
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.
Also, read a lot of code reviews. Just like reading a lot of code is helpful for becoming a better developer, reading a lot of reviews is helpful for becoming a better reviewer.