-
Notifications
You must be signed in to change notification settings - Fork 37.8k
doc: Explain that feedback needs to be addressed #24433
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
The head ref may contain hidden characters: "2202-doc-contrib-\u{1F406}"
Conversation
fad2343
to
fa39841
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK, Approach ACK
Generally these changes are improvements, just a couple of nits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK fad2343
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.
fa39841
to
fa694f6
Compare
ACK fa694f6 |
it, but you should express so in a reply. If there is outstanding feedback and | ||
you are not actively working on it, your pull request may be closed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example: #13360 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK fa694f6
This PR improves docs. I was not sure about one line but example provided by PR author looks okay.
ACK fa694f6 |
ACK fa694f6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK fa694f6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK fa694f6
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.