Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Feb 23, 2022

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.

@fanquake fanquake added the Docs label Feb 23, 2022
Copy link

@michaelfolkson michaelfolkson left a 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.

Copy link
Contributor

@shaavan shaavan left a 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.

@michaelfolkson
Copy link

ACK fa694f6

Comment on lines +196 to +197
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.
Copy link

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example: #13360 (comment)

Copy link

@ghost ghost left a 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.

@Sjors
Copy link
Member

Sjors commented Feb 24, 2022

ACK fa694f6

@jamesob
Copy link
Contributor

jamesob commented Feb 24, 2022

ACK fa694f6

Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa694f6

@bitcoin bitcoin deleted a comment from Pragmatic2021 Feb 24, 2022
@bitcoin bitcoin deleted a comment from Pragmatic2021 Feb 24, 2022
Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa694f6

@fanquake fanquake merged commit 0dc1002 into bitcoin:master Feb 25, 2022
@maflcko maflcko deleted the 2202-doc-contrib-🐆 branch February 25, 2022 12:48
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 25, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Jun 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants