@@ -237,24 +237,35 @@ request. Typically reviewers will review the code for obvious errors, as well as
237
237
test out the patch set and opine on the technical merits of the patch. Project
238
238
maintainers take into account the peer review when determining if there is
239
239
consensus to merge a pull request (remember that discussions may have been
240
- spread out over GitHub, mailing list and IRC discussions). The following
240
+ spread out over GitHub, mailing list and IRC discussions).
241
+
242
+ #### Conceptual Review
243
+
244
+ A review can be a conceptual review, where the reviewer leaves a comment
245
+ * ` Concept (N)ACK ` , meaning "I do (not) agree in the general goal of this pull
246
+ request",
247
+ * ` Approach (N)ACK ` , meaning ` Concept ACK ` , but "I do (not) agree with the
248
+ approach of this change".
249
+
250
+ A ` NACK ` needs to include a rationale why the change is not worthwhile.
251
+ NACKs without accompanying reasoning may be disregarded.
252
+
253
+ #### Code Review
254
+
255
+ After conceptual agreement on the change, code review can be provided. It is
256
+ starting with ` ACK BRANCH_COMMIT ` , where ` BRANCH_COMMIT ` is the top of the
257
+ topic branch. The review is followed by a description of how the reviewer did
258
+ the review. The following
241
259
language is used within pull-request comments:
242
260
243
- - (t)ACK means "I have tested the code and I agree it should be merged ", involving
261
+ - "I have tested the code", involving
244
262
change-specific manual testing in addition to running the unit and functional
245
263
tests, and in case it is not obvious how the manual testing was done, it should
246
264
be described;
247
- - NACK means "I disagree this should be merged", and must be accompanied by
248
- sound technical justification (or in certain cases of copyright/patent/licensing
249
- issues, legal justification). NACKs without accompanying reasoning may be
250
- disregarded;
251
- - utACK means "I have not tested the code, but I have reviewed it and it looks
265
+ - "I have not tested the code, but I have reviewed it and it looks
252
266
OK, I agree it can be merged";
253
- - Concept ACK means "I agree in the general principle of this pull request";
254
267
- Nit refers to trivial, often non-blocking issues.
255
268
256
- Reviewers should include the commit hash which they reviewed in their comments.
257
-
258
269
Project maintainers reserve the right to weigh the opinions of peer reviewers
259
270
using common sense judgement and also may weight based on meritocracy: Those
260
271
that have demonstrated a deeper commitment and understanding towards the project
0 commit comments