Those are good things to consider in review, but I maintain that the answer might be "no" to one or more of those questions and still be acceptable.
I'm old enough to have worked in the pre-code-review era. Things were fine. People still learned from each other, software could still be great or terrible, etc. It wasn't appreciably worse or better than things are today.
> An implicit question in several of the above is "will this set a good example for future contributions?"
Which in my experience can be an almost circular requirement. What do you consider a good example? As perfect as perfect can be? Rapid development? Extreme pragmatism?
The more experienced I get, the less I complain about in code review, especially when reviewing for a more junior dev, and especially for frequent comitters. People can only get so much out of any single code review, and any single commit can only do so much damage.
Put another way, code review is also about a level of trust. Will the committer be around next week? Are they on the same team as me? If yes, give them some leeway to commit incremental work and make improvements later. Not all incremental work need occur pre-commit. Mention areas for improvement, sure, but don't go overboard as a gatekeeper.
Things are obviously going to be different when reviewing code from what amounts to a stranger on a mission critical piece of code, etc.
> Put another way, code review is also about a level of trust. Will the committer be around next week? Are they on the same team as me? If yes, give them some leeway to commit incremental work and make improvements later. Not all incremental work need occur pre-commit. Mention areas for improvement, sure, but don't go overboard as a gatekeeper.
I think this is very important, especially the part about incremental improvement. too many see development as laying concrete where it has to be perfect rather than as an ongoing process.
and personally the only thing I find PR's good for is ensuring jackasses aren't doing stupid shit. And by stupid shit here I mean things like using floats for currency (I caught that w/i the last year), things of that nature.
But my preference is to work with people I can trust and at that point I don't give a crap about a PR or a code review.
I'm old enough to have worked in the pre-code-review era. Things were fine. People still learned from each other, software could still be great or terrible, etc. It wasn't appreciably worse or better than things are today.
> An implicit question in several of the above is "will this set a good example for future contributions?"
Which in my experience can be an almost circular requirement. What do you consider a good example? As perfect as perfect can be? Rapid development? Extreme pragmatism?
The more experienced I get, the less I complain about in code review, especially when reviewing for a more junior dev, and especially for frequent comitters. People can only get so much out of any single code review, and any single commit can only do so much damage.
Put another way, code review is also about a level of trust. Will the committer be around next week? Are they on the same team as me? If yes, give them some leeway to commit incremental work and make improvements later. Not all incremental work need occur pre-commit. Mention areas for improvement, sure, but don't go overboard as a gatekeeper.
Things are obviously going to be different when reviewing code from what amounts to a stranger on a mission critical piece of code, etc.