doc: Explain that feedback needs to be addressed #24433

MarcoFalke commented at 6:26 PM on February 23, 2022: member

Generally, the pull request author is expected to reply to all comments or iterate the code before merge. Of course, it is allowed to reject feedback, but it should not be done by silently ignoring it.

Clarify this in the docs.

Also, some minor copy edits.

doc: Add link to release-process.md in CONTRIBUTING.md fa2b65b358

doc: Move peer-review paragraph to right section fa0819eea3

fanquake added the label Docs on Feb 23, 2022

in CONTRIBUTING.md:194 in fad2343087 outdated

 188 | @@ -189,9 +189,15 @@ in the body of the pull request to indicate tasks are pending.
 189 |  
 190 |  At this stage, one should expect comments and review from other contributors. You
 191 |  can add more commits to your pull request by committing them locally and pushing
 192 | -to your fork until you have satisfied all feedback.
 193 | +to your fork.
 194 |  
 195 | -Note: Code review is a burdensome but important part of the development process, and as such, certain types of pull requests are rejected. In general, if the **improvements** do not warrant the **review effort** required, the PR has a high chance of being rejected. It is up to the PR author to convince the reviewers that the changes warrant the review effort, and if reviewers are "Concept NACK'ing" the PR, the author may need to present arguments and/or do research backing their suggested changes.
 196 | +As the pull request author you are expected to address any review comments

brunoerg commented at 10:02 PM on February 23, 2022:

"As the pull request author you are expected to address any review comments before your pull request is merged" - What do you mean by 'address'?

unknown commented at 3:01 AM on February 24, 2022:

If @MarcoFalke responds to this comment and explains the meaning of 'address' in this context, it will be 'addressing' review comments of this pull request.

In other pull requests it can be related to code and sometimes reviewers may not agree with certain changes or rationale provided for the code.

brunoerg commented at 11:26 AM on February 24, 2022:

Make sense. I thought it could sound 'implement' what it is not true since we may not agree with all feedbacks. But it sounds like 'reply' or 'consider' what makes sense. Thank you for explaining, just a doubt related to this meaning.

MarcoFalke commented at 12:28 PM on February 24, 2022:

Thanks, changed a bit.

MarcoFalke force-pushed on Feb 24, 2022

in CONTRIBUTING.md:195 in fa39841fce outdated

 188 | @@ -189,9 +189,15 @@ in the body of the pull request to indicate tasks are pending.
 189 |  
 190 |  At this stage, one should expect comments and review from other contributors. You
 191 |  can add more commits to your pull request by committing them locally and pushing
 192 | -to your fork until you have satisfied all feedback.
 193 | +to your fork.
 194 |  
 195 | -Note: Code review is a burdensome but important part of the development process, and as such, certain types of pull requests are rejected. In general, if the **improvements** do not warrant the **review effort** required, the PR has a high chance of being rejected. It is up to the PR author to convince the reviewers that the changes warrant the review effort, and if reviewers are "Concept NACK'ing" the PR, the author may need to present arguments and/or do research backing their suggested changes.
 196 | +As the pull request author you are expected to reply to any review comments
 197 | +before your pull request is merged. You may interate the code or reject the

michaelfolkson commented at 12:32 PM on February 24, 2022:

I think "interate" is a typo. What is this supposed to be, "iterate"? I think "update" or "adjust" would be better here?

MarcoFalke commented at 12:48 PM on February 24, 2022:

Thanks, fixed

in CONTRIBUTING.md:194 in fa39841fce outdated

 188 | @@ -189,9 +189,15 @@ in the body of the pull request to indicate tasks are pending.
 189 |  
 190 |  At this stage, one should expect comments and review from other contributors. You
 191 |  can add more commits to your pull request by committing them locally and pushing
 192 | -to your fork until you have satisfied all feedback.
 193 | +to your fork.
 194 |  
 195 | -Note: Code review is a burdensome but important part of the development process, and as such, certain types of pull requests are rejected. In general, if the **improvements** do not warrant the **review effort** required, the PR has a high chance of being rejected. It is up to the PR author to convince the reviewers that the changes warrant the review effort, and if reviewers are "Concept NACK'ing" the PR, the author may need to present arguments and/or do research backing their suggested changes.
 196 | +As the pull request author you are expected to reply to any review comments

michaelfolkson commented at 12:36 PM on February 24, 2022:

Here it is "as the pull request author you are". But later it moves from "you" back to the PR "author may need to..."

Try to be consistent? Either it is all "you should" or "the PR author should". I think I prefer the latter here. It gets confusing if it is not clear whether "you" is the PR author or the PR reviewer.

MarcoFalke commented at 12:49 PM on February 24, 2022:

This section uses "you", the others do not, so I stuck to that.

michaelfolkson commented at 12:37 PM on February 24, 2022: contributor

Concept ACK, Approach ACK

Generally these changes are improvements, just a couple of nits.

shaavan approved

shaavan commented at 12:39 PM on February 24, 2022: contributor

ACK fad23430870f6acf3b343302a02aae97131db689

Going commit wise:

Adding a link to “release cycle” documentation and mentioning it at the beginning of documentation removes the need for a separate Release Policy section. Also, a reader would better understand a release cycle and the role of a “lead maintainer” in it.
I agree that the note in the Address Feedback section better belongs to the Peer Review section.
Removing “until you have satisfied all feedback.” increases the sentence’s brevity. Also, the added paragraph makes the reader understand the need to address the feedback on their PRs. This is an essential point and habit that every new contributor should follow.

doc: Explain that feedback needs to be addressed fa694f61ab

MarcoFalke force-pushed on Feb 24, 2022

michaelfolkson commented at 12:58 PM on February 24, 2022: contributor

ACK fa694f61ab21cb973843e5234a6aeacd4a957e05

in CONTRIBUTING.md:197 in fa694f61ab

 193 | +to your fork.
 194 | +
 195 | +You are expected to reply to any review comments before your pull request is
 196 | +merged. You may update the code or reject the feedback if you do not agree with
 197 | +it, but you should express so in a reply. If there is outstanding feedback and
 198 | +you are not actively working on it, your pull request may be closed.

unknown commented at 1:28 PM on February 24, 2022:

Last sentence can be interpreted in different ways and used to close pull requests. I am not sure about this. Do you have any examples or can this be rephrased to make things clear?