mirror of
https://github.com/dashpay/dash.git
synced 2024-12-24 19:42:46 +01:00
Merge #16149: doc: Rework section on ACK in CONTRIBUTING.md
fac5ddfc57 doc: Rework section on ACK (MarcoFalke) Pull request description: `utACK` and `t(ested) ACK` are deprecated and will be removed in the next major release. Please use the more generic `ACK` and include an explanation of what was reviewed. There was a related discussion in http://diyhpl.us/wiki/transcripts/bitcoin-core-dev-tech/2019-06-05-code-review/ section `The author could offer a guide for review`. ACKs for commit fac5dd: moneyball: ACK fac5ddfc5796022601af2c17fd0b158b2c465cba Tree-SHA512: 29177e8d96aeba055b5cad6d99be3ca1be0c61af0fdc90f70a3136872c9ad6201a02f63fbac78b90b8a56b4c06af304f2583d52a94fdd954fdcc7ad0552b9ef8
This commit is contained in:
parent
184c0e0f8f
commit
7066614d6c
@ -210,7 +210,22 @@ request. Typically reviewers will review the code for obvious errors, as well as
|
||||
test out the patch set and opine on the technical merits of the patch. Project
|
||||
maintainers take into account the peer review when determining if there is
|
||||
consensus to merge a pull request (remember that discussions may have been
|
||||
spread out over GitHub, mailing list and IRC discussions). The following
|
||||
spread out over GitHub, mailing list and IRC discussions).
|
||||
|
||||
#### Conceptual Review
|
||||
|
||||
A review can be a conceptual review, where the reviewer leaves a comment
|
||||
* `Concept (N)ACK`, meaning "I do (not) agree in the general goal of this pull
|
||||
request",
|
||||
* `Approach (N)ACK`, meaning `Concept ACK`, but "I do (not) agree with the
|
||||
approach of this change".
|
||||
|
||||
#### Code Review
|
||||
|
||||
After conceptual agreement on the change, code review can be provided. It is
|
||||
starting with `ACK BRANCH_COMMIT`, where `BRANCH_COMMIT` is the top of the
|
||||
topic branch. The review is followed by a description of how the reviewer did
|
||||
the review. The following
|
||||
language is used within pull-request comments:
|
||||
|
||||
- (t)ACK means "I have tested the code and I agree it should be merged", involving
|
||||
@ -222,12 +237,8 @@ language is used within pull-request comments:
|
||||
issues, legal justification). NACKs without accompanying reasoning may be
|
||||
disregarded;
|
||||
- utACK means "I have not tested the code, but I have reviewed it and it looks
|
||||
OK, I agree it can be merged";
|
||||
- Concept ACK means "I agree in the general principle of this pull request";
|
||||
- Nit refers to trivial, often non-blocking issues.
|
||||
|
||||
Reviewers should include the commit hash which they reviewed in their comments.
|
||||
|
||||
Project maintainers reserve the right to weigh the opinions of peer reviewers
|
||||
using common sense judgement and also may weight based on meritocracy: Those
|
||||
that have demonstrated a deeper commitment and understanding towards the project
|
||||
|
Loading…
Reference in New Issue
Block a user