Create code review guidelines

[chandlerc]

See proposal document for details.

The actual copy in the code review guidelines is fairly rough and may need some helpful improvements. =] I don't claim to be the best at this, and have been leaning very heavily on existing guidance I have found while trying to distill enough of it into a single document for Carbon's usage that we don't have to fully rely on external links. I suspect there are more improvements needed.

RFC thread
Decision thread

[jonmeow]

Still happy with this.

[zygoloid]

I find this sentence hard to understand. I think this means:

For evolution proposals of any kind, when members of the deciding team for that proposal provide a decision, they should explicitly note any review comments that blocked them from completing code review.

Is that the intended interpretation? If so, that seems a little surprising to me, as I would think we should generally try to not reach the decision phase if there's some outstanding comment of sufficient severity that the proposal couldn't be reviewed in full. And to the extent that there are outstanding comments of this kind, I'd expect them to lead to a "no" vote on the decision, which would make the code review comments immaterial because the PR isn't going to be merged in that form anyway.

Perhaps I'm misunderstanding what "completing the code review" intends in this context. I would also like clarity of what "team member" means -- which team?

[chandlerc]

Sorry, Jon also found this confusing. I accepted his suggestion, but also substantially reworded this to hopefully be more clear.

FWIW, my intent is different from your interpretation. I've tried to both make it clear which team is relevant and what my intent is here, but please let me know if still not working.

[jonmeow]

@zygoloid Just as an example of the issue I was hoping to address, on the goals PR, gromer and chandlerc had left "changes requested" as their review state, pre-dating the decision -- I cleared that before committing. In addition, it's essentially infeasible for me to go to a decision with explicit confirmation on each comment thread: I see a typical flow where a core team member comments, I respond, and that's it -- they don't "explicitly confirm they're happy" as stated on line 87.

Thus, my argument that in a proposal with many people commenting, we should not expect that standard. Instead, it's the core team's consensus judgment when things are done. Or we could switch advice to work better for proposal PRs, but I actually think explicit confirmation from commenters should work fine on non-proposal PRs.

[zygoloid]

Thanks, the rewrite has made the intent here much clearer to me.

I think we should be pushing more for people to resolve their comment threads once their concerns are resolved to the extent that's not already happening, but it also seems reasonable to say that we consider an "Affirm" to mean all outstanding comments from that person should be considered resolved, so this rule seems fine to me.

[zygoloid]

I come away from the end of this paragraph thinking that we should allow any change that makes the aggregate quality of the repository better, even if (say) it contains a good change with a bad rider, where the badness of the latter is less extreme than the goodness of the former. There's a tricky line to draw here, and we want progress rather than perfection, but (for example) I think I would still want people to push back on a change that fixes a major bug if it flagrantly violates our style guide, even if the change would improve the overall health of the project.

Maybe instead of the final sentence, something like "If the change is an overall improvement in the absence of the suggestion, and doesn't significantly regress the health of any aspect of the project, it may be fine for it to move forward as-is." would reflect that?

If you meant that we should allow significant local regressions in order to achieve a more significant global improvement, I think that would be worth calling out explicitly.

[chandlerc]

I feel like this is striving for a very precise rubric and I'm not sure we can get there...

For the scenario you raise, I totally agree. The fact that a bug is being fixed doesn't exempt the change from basic requirements around style, testing, etc. However, I'm not sure about your suggestion. What about cases where the best way to address a nasty bug is to take on technical debt? I think there are real cases where that will be necessary, and I think they will in some cases "significantly regress the health" of one aspect of the project in order to address bugs.

A concrete example that came up in LLVM some years ago: there was a long-standing bug in LLVM that finally ended up breaking some real-world code for one large user. They had a product team blocked because of this. It was a major issue. However, the infrastructure was simply not present to fix it fully. That would take months. So they landed a patch that didn't actually fix the bug, but avoided it in the case that impacted their team. However, it introduced yet another bug for another major user. They in turn proposed a patch that actively violated the LLVM semantic contract in a narrow case in order to avoid this bug in turn. Should we accept neither of these patches? Only the first? Both? =/ I think the correct answer in retrospect was to accept both, even though the first created clear technical debt, and the second worsened it by creating an active contradiction in the semantic model. Eventually, we ripped out the broken infrastructure and put working infrastructure in place.

Long story, sorry. The point is, I'm worried that we're going to end up with rules that arbitrarily suggest only one path here is correct, when I think the goal should be a judgement call based on the particular circumstance... I'm unsure how to phrase this in a way that helps though.... Let me know if you have other ideas, I'll keep thinking of ways to phrase this better too...

[zygoloid]

OK, so it seems like the intent is to leave open a broad choice here; sometimes we do want to allow local regressions to enable global progress, and sometimes (I would say usually) we don't. I think the wording as-is is fine for that: it contains enough qualifiers ("consider", "may be") that it's leaving the choice open to the reviewer, which sounds like it is what you wanted. That works for me. Thanks for the story :)

[zygoloid]

Generally I'm happy with this direction, but there are a couple of places where the intent of the proposal is not entirely clear to me.

[jonmeow]

Thanks for the bullet, hopefully it's not too wordy for people. :)

[jonmeow]

@zygoloid Just as an example of the issue I was hoping to address, on the goals PR, gromer and chandlerc had left "changes requested" as their review state, pre-dating the decision -- I cleared that before committing. In addition, it's essentially infeasible for me to go to a decision with explicit confirmation on each comment thread: I see a typical flow where a core team member comments, I respond, and that's it -- they don't "explicitly confirm they're happy" as stated on line 87.

Thus, my argument that in a proposal with many people commenting, we should not expect that standard. Instead, it's the core team's consensus judgment when things are done. Or we could switch advice to work better for proposal PRs, but I actually think explicit confirmation from commenters should work fine on non-proposal PRs.

[zygoloid]

LGTM

[zygoloid]

OK, so it seems like the intent is to leave open a broad choice here; sometimes we do want to allow local regressions to enable global progress, and sometimes (I would say usually) we don't. I think the wording as-is is fine for that: it contains enough qualifiers ("consider", "may be") that it's leaving the choice open to the reviewer, which sounds like it is what you wanted. That works for me. Thanks for the story :)