/bitcoin

Add PR description to merge commit in github-merge.py #10786

Merged
merged 1 commit into from Jul 11, 2017
+2 −0

Conversation

Reviewers

@promag promag

Assignees
No one assigned
Labels
Scripts and tools
Projects
None yet
Milestone
No milestone
5 participants
@sipa @MarcoFalke @promag @jonasschnelli @laanwj
@sipa
Owner

sipa commented Jul 10, 2017

There is often some context given in PR descriptions that is missing from commits, and it may be worthwhile to retain that information in our history in git. This PR adds that information to the merge commit when created through github-merge.py.

We should also encourage people to provide as much information as possible in the PR commits themselves, but I believe that is an orthogonal issue. Individual commits don't need to have a description of the overall goal of a PR.
@MarcoFalke
Member

MarcoFalke commented Jul 10, 2017

contrib/devtools/github-merge.py
@@ -175,6 +175,7 @@ def main():
if info is None:
exit(1)
title = info['title'].strip()
+ body = ' ' + info['body'].replace('\n', '\n ')
@promag

promag Jul 10, 2017

Contributor

I think here should be body = info['body'].strip() like the above. And the body indentation should be below.

@sipa

sipa Jul 10, 2017

Owner

Agree, fixed.

@promag
Contributor

promag commented Jul 10, 2017

ACK 475c08c.

For the record, after this, the PR description must be part of the review process. Don't forget that there can be typos, markdown, links for temporary resources, etc that will be written in stone. Maybe for future improvement is to print the body and prompt for confirmation.
@jonasschnelli
Member

jonasschnelli commented Jul 11, 2017

Great idea.
Tested ACK 475c08c (libbtc/libbtc@7c6168c)

jonasschnelli added the Scripts and tools label Jul 11, 2017

@laanwj
Owner

laanwj commented Jul 11, 2017

Concept ACK

@laanwj laanwj merged commit 475c08c into bitcoin:master Jul 11, 2017
1 check passed

1 check passed

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

@laanwj laanwj added a commit that referenced this pull request Jul 11, 2017

@laanwj laanwj Merge #10786: Add PR description to merge commit in github-merge.py 
475c08c Add PR description to merge commit in github-merge.py (Pieter Wuille)

Pull request description:

  There is often some context given in PR descriptions that is missing from commits, and it may be worthwhile to retain that information in our history in git. This PR adds that information to the merge commit when created through `github-merge.py`.

  We should also encourage people to provide as much information as possible in the PR commits themselves, but I believe that is an orthogonal issue. Individual commits don't need to have a description of the overall goal of a PR.

Tree-SHA512: cbae46ec24ce911744e11a07e5d51895d3acc79659db306d8124304ef8e422bba99001ee2a741b08c92a5eb39d9c3f6b723622b20d6553ca5cfa0a8de866194c
This commit was signed with a verified signature.
@laanwj laanwj Wladimir J. van der Laan
GPG key ID: 1E4AED62986CD25D Learn about signing commits
ca4c545
@jonasschnelli
Member

jonasschnelli commented Jul 11, 2017

I guess we are fine with HTML code in the PR description (=> goes then into git commit message)?
PRs like #10769 may include images (via HTML <img>code).

ryanofsky referenced this pull request Jul 12, 2017

Merged

[wallet] [tests] Add listwallets RPC, include wallet name in `getwalletinfo` and add multiwallet test #10604

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