New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Dec 14, 2015

Conversation

Projects
None yet
8 participants
@pstratem
Contributor

pstratem commented Dec 8, 2015

No description provided.

@luke-jr

View changes

Show outdated Hide outdated CONTRIBUTING.md
@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Dec 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.

Contributor

dcousens commented Dec 8, 2015

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

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Dec 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

Contributor

pstratem commented Dec 8, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Dec 8, 2015

Contributor

👍

Contributor

dcousens commented Dec 8, 2015

👍

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Dec 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.

Member

laanwj commented Dec 8, 2015

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

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Dec 8, 2015

Contributor

@laanwj sure thus, SHOULD instead of MUST

Contributor

pstratem commented Dec 8, 2015

@laanwj sure thus, SHOULD instead of MUST

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Dec 8, 2015

Member

Ok, concept ACK, agree on the style nits.

Member

laanwj commented Dec 8, 2015

Ok, concept ACK, agree on the style nits.

@laanwj laanwj added the Docs label Dec 8, 2015

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Dec 8, 2015

Contributor

@luke-jr is that acceptable?

Contributor

pstratem commented Dec 8, 2015

@luke-jr is that acceptable?

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Dec 8, 2015

Member

Sure

Member

luke-jr commented Dec 8, 2015

Sure

@fanquake

This comment has been minimized.

Show comment
Hide comment
@fanquake

fanquake Dec 9, 2015

Member

ACK, after updating the commit message. It still mentions id rather than hash.

Member

fanquake commented Dec 9, 2015

ACK, after updating the commit message. It still mentions id rather than hash.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Dec 10, 2015

Member

ACK 7ad0537

Member

MarcoFalke commented Dec 10, 2015

ACK 7ad0537

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik
Contributor

paveljanik commented Dec 10, 2015

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd
Contributor

petertodd commented Dec 11, 2015

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Dec 11, 2015

Contributor

@fanquake nit picked

Contributor

pstratem commented Dec 11, 2015

@fanquake nit picked

@fanquake

This comment has been minimized.

Show comment
Hide comment
@fanquake
Member

fanquake commented Dec 11, 2015

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke
Member

MarcoFalke commented Dec 14, 2015

ACK e1030dd

@laanwj laanwj merged commit e1030dd into bitcoin:master Dec 14, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Dec 14, 2015

Merge pull request #7185
e1030dd Note that reviewers should mention the commit hash of the commits they reviewed. (Patrick Strateman)
@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Dec 18, 2015

Contributor

another great example of why this is necessary #6451

Contributor

pstratem commented Dec 18, 2015

another great example of why this is necessary #6451

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jan 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment