Note that reviewers should mention the id of the commits they reviewed. #7185

pull pstratem wants to merge 1 commits into bitcoin:master from pstratem:2015-12-07-contributingackcommit changing 1 files +2 −0

pstratem commented at 12:14 AM on December 8, 2015: contributor

No description provided.

in CONTRIBUTING.md:None in 87bcd8a3b0 outdated

  94 | @@ -95,6 +95,8 @@ Anyone may participate in peer review which is expressed by comments in the pull
  95 |    - Concept ACK means "I agree in the general principle of this pull request";
  96 |    - Nit refers to trivial, often non-blocking issues.
  97 |  
  98 | +Reviewers should include the id of the commits which they reviewed in their comments

luke-jr commented at 1:15 AM on December 8, 2015:

commit hash*

paveljanik commented at 6:06 AM on December 8, 2015:

And a dot at the end of the sentence.

luke-jr commented at 6:46 AM on December 8, 2015:

How about this dot: ᣟ

paveljanik commented at 7:38 AM on December 8, 2015:

I do not see any dot there Luke 8)

dcousens commented at 1:57 AM on December 8, 2015: contributor

Not that it should be trusted verbatim (I guess?), but if any changes occur after a comment, then the changed commits will be moved below the original comment.
pstratem commented at 2:12 AM on December 8, 2015: contributor

@dcousens yes but without a reference to which commits were ACK'd it's impossible to tell what if anything was changed

for an example of this see #3154
dcousens commented at 7:31 AM on December 8, 2015: contributor

:+1:
laanwj commented at 9:21 AM on December 8, 2015: member

Agree that this is useful in some cases, but as long as this is a manual step I don't really want to force it on everyone.
pstratem commented at 9:26 AM on December 8, 2015: contributor

@laanwj sure thus, SHOULD instead of MUST
laanwj commented at 12:56 PM on December 8, 2015: member

Ok, concept ACK, agree on the style nits.
laanwj added the label Docs and Output on Dec 8, 2015
pstratem commented at 9:07 PM on December 8, 2015: contributor

@luke-jr is that acceptable?
luke-jr commented at 11:36 PM on December 8, 2015: member

Sure
fanquake commented at 8:02 AM on December 9, 2015: member

ACK, after updating the commit message. It still mentions id rather than hash.
MarcoFalke commented at 5:43 PM on December 10, 2015: member

ACK 7ad0537
paveljanik commented at 7:58 PM on December 10, 2015: contributor

ACK https://github.com/pstratem/bitcoin/commit/7ad0537ee00f5bf792b0156de06a3207e4172009
petertodd commented at 6:40 AM on December 11, 2015: contributor

ACK https://github.com/pstratem/bitcoin/commit/7ad0537ee00f5bf792b0156de06a3207e4172009
pstratem commented at 6:44 AM on December 11, 2015: contributor

@fanquake nit picked
Note that reviewers should mention the commit hash of the commits they reviewed. e1030dddab
fanquake commented at 7:49 AM on December 11, 2015: member

ACK https://github.com/pstratem/bitcoin/commit/e1030dddab11553d2854c1f466e5757d9815bfb8
MarcoFalke commented at 6:57 AM on December 14, 2015: member

ACK e1030dd
laanwj merged this on Dec 14, 2015
laanwj closed this on Dec 14, 2015
laanwj referenced this in commit ea0f5a2b04 on Dec 14, 2015
pstratem commented at 12:45 PM on December 18, 2015: contributor

another great example of why this is necessary #6451
luke-jr referenced this in commit 8551b4fe80 on Jan 13, 2016
luke-jr referenced this in commit 6307beb09f on Jan 13, 2016
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels