As with a lot of things in life, it depends.
I use 1-5 depending on the repo, who made the change, what the change is about, and how involved I am in the project.
Though the “time-frame” idea of #4 is usually replaced by conversations if it’s a coworker, as it’s more effective.
Q: about #3, do you mean on code that is already merged / committed to the default branch?
Added a few details in the post. Of course it depends, but let’s say you’re the team lead and you have to fix a general rule (otherwise no one is going to do them) which one you’re more likely to go for? e.g. if you choose (2), it’s up to every single member.
Q: about #3
yes already merged, updated the post.
The way I see it, for any code review there are going to be different levels of recommendation regarding the comments. When I review, I try to make it clear what’s optional (/ nitpick) and what I’d really like to see fixed before I can approve it.
So even making some assumptions, I can’t choose between 4 and 5 because optional and “less optional” changes are often in a same PR.
The only one I haven’t done much of is #3. That one looks better if one has questions about code that was already reviewed, merged, and it’s likely in production.
mattreb
Eager Eagle