Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
laanwj merged 1 commit into from
Dec 14, 2015
Merged

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

laanwj merged 1 commit into from
Dec 14, 2015

Conversation

@pstratem
Copy link
Contributor

@pstratem pstratem commented Dec 8, 2015

No description provided.

luke-jr
luke-jr reviewed Dec 8, 2015
View reviewed changes
CONTRIBUTING.md Outdated
Copy link
Member

@luke-jr luke-jr Dec 8, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commit hash*

Copy link
Contributor

@paveljanik paveljanik Dec 8, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And a dot at the end of the sentence.

Copy link
Member

@luke-jr luke-jr Dec 8, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this dot: ᣟ

Copy link
Contributor

@paveljanik paveljanik Dec 8, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see any dot there Luke 8)

@dcousens
Copy link
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
Copy link
Contributor Author

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
Copy link
Contributor

dcousens commented Dec 8, 2015

👍

@laanwj
Copy link
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
Copy link
Contributor Author

pstratem commented Dec 8, 2015

@laanwj sure thus, SHOULD instead of MUST

@laanwj
Copy link
Member

laanwj commented Dec 8, 2015

Ok, concept ACK, agree on the style nits.

@laanwj laanwj added the Docs label Dec 8, 2015
@pstratem
Copy link
Contributor Author

pstratem commented Dec 8, 2015

@luke-jr is that acceptable?

@luke-jr
Copy link
Member

luke-jr commented Dec 8, 2015

Sure

@fanquake
Copy link
Member

fanquake commented Dec 9, 2015

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

@maflcko
Copy link
Member

maflcko commented Dec 10, 2015

ACK 7ad0537

@paveljanik
Copy link
Contributor

paveljanik commented Dec 10, 2015

ACK pstratem@7ad0537

1 similar comment
@petertodd
Copy link
Contributor

petertodd commented Dec 11, 2015

ACK pstratem@7ad0537

@pstratem
Copy link
Contributor Author

pstratem commented Dec 11, 2015

@fanquake nit picked

@fanquake
Copy link
Member

fanquake commented Dec 11, 2015

ACK pstratem@e1030dd

@maflcko
Copy link
Member

maflcko commented Dec 14, 2015

ACK e1030dd

@laanwj laanwj merged commit e1030dd into bitcoin:master Dec 14, 2015
laanwj added a commit that referenced this pull request Dec 14, 2015
@laanwj
Merge pull request #7185
ea0f5a2 
e1030dd Note that reviewers should mention the commit hash of the commits they reviewed. (Patrick Strateman)
@pstratem
Copy link
Contributor Author

pstratem commented Dec 18, 2015

another great example of why this is necessary #6451

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jan 13, 2016
@pstratem @luke-jr
Note that reviewers should mention the commit hash of the commits the…
6307beb 
…y reviewed.

Github-Pull: bitcoin#7185
Rebased-From: e1030dd
@JeremyRand JeremyRand mentioned this pull request Jun 24, 2017
Merged
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Docs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@pstratem @dcousens @laanwj @luke-jr @fanquake @maflcko @paveljanik @petertodd