search

google / eng-practices

Google's Engineering Practices documentation
https://google.github.io/eng-practices/
Other
20.03k stars 1.95k forks source link

Resolving comments #35

Open CyberMew opened 4 years ago

CyberMew commented 4 years ago

Not sure if this was addressed in the article yet. Who should be in charge of resolving the comments? Should the reviewee resolve it on their own when they think it is fixed, or should the reviewer come back and verify the changes and resolve the thread? Who has the final decision?

ninachen commented 3 years ago

(Somehow missed this issue, apologies.) I think either is fine as a cultural norm as long as everyone is on the same page about it.

If you want to know what Google does, we recommend that authors (reviewees) resolve comments when they've addressed the comment and leave the comment unresolved if still needs action. We do have a section that addresses this, although it is not mirrored to github because most of the content is about specifics of what buttons to click in Google's internal code review tool.

I've copied an abridged version below:

Responding to Comments {#responding}

Unresolved comments are highlighted in the ... UI and await a response. You are expected to resolve all comments before submitting a CL.

[If you have applied the suggestion, mark the comment as resolved.]

[If you have NOT applied the suggestion], reply to the comment with an explanation of what you chose to do — and why. See the section on thinking for yourself. The practice of explaining your decisions will reduce the number of iterations it takes to review your code and will provide useful context to reviewers and future code readers.

Prefer leaving comments unresolved when further discussion or action is likely necessary.

I'm not immeeediately sure whether it's possible to insert a public-friendly version of this "Responding to Comments" section in our docs without munging the internal version.

ninachen commented 3 years ago

And of course, even in the abridged form, this assumes that wherever the code review is occuring supports resolving vs. unresolving comments.