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: ACKfac5ddfc57
Tree-SHA512: 29177e8d96aeba055b5cad6d99be3ca1be0c61af0fdc90f70a3136872c9ad6201a02f63fbac78b90b8a56b4c06af304f2583d52a94fdd954fdcc7ad0552b9ef8
This commit is contained in:
commit
2ea8ebd211
1 changed files with 21 additions and 10 deletions
|
@ -237,24 +237,35 @@ 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".
|
||||
|
||||
A `NACK` needs to include a rationale why the change is not worthwhile.
|
||||
NACKs without accompanying reasoning may be disregarded.
|
||||
|
||||
#### 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
|
||||
- "I have tested the code", involving
|
||||
change-specific manual testing in addition to running the unit and functional
|
||||
tests, and in case it is not obvious how the manual testing was done, it should
|
||||
be described;
|
||||
- NACK means "I disagree this should be merged", and must be accompanied by
|
||||
sound technical justification (or in certain cases of copyright/patent/licensing
|
||||
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
|
||||
- "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 a new issue