I am a junior developer and my latest feedback was that one of the main skills I should develop is to make better, more well-thought, critic and deep code reviews (including of PRs from more senior developers).
Any tips on how to improve this?
Would a checklist help? Have a clear process on what to review first?
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.
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.
Ask for examples of good reviews to emulate. Look at the PR without comments first, and give your own review. Then compare with the original review, and see what areas you emphasized more, and emphasized less than the original review. Talk with the original reviewer, and ask about mindset behind why they asked for the changes they did.
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.
Unless your current reviews are really superficial probably not. You can only review at your level of understanding, so if they're asking for more depth to your code reviews you probably need to develop a deeper understanding of the architecture and design of your codebase. A checklist would do the opposite of that in most cases.
Any tips on how to improve this?
Would a checklist help? Have a clear process on what to review first?