Skip to content

Conversation

@laanwj
Copy link
Member

@laanwj laanwj commented Jan 19, 2016

This is meant to be a direct translation of the bash script.

The only major difference is that it retrieves the PR title from github, thus creating pull messages like:

Merge pull request #12345: Expose transaction temperature over RPC

[skip ci]

@laanwj laanwj force-pushed the 2016_01_python_github_merge branch from 340310a to b86484c Compare January 19, 2016 09:19
@jonasschnelli
Copy link
Contributor

Nice!
Tested ACK (on a different repo: BitBoxSwiss/dbb-app@a592638).

Thought for future additions:

  • show the head SHA256 hash from the PR (possible short compare against the one you have ACKed)
  • short command to diff the PR (temp bash alias instead of typing git diff HEAD~1?)

@maflcko
Copy link
Member

maflcko commented Jan 19, 2016

utACK b86484c

Copy link
Member

Choose a reason for hiding this comment

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

Could make sense to "sanitize" the title such that it fits in the git(hub) subject line.

That is, return title[0:min(80, len(title)) - len("Merge pull request #xxxx: "]

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know - I don't like it cut off.
At least github tends to handle long title lines already by adding ... and continuing on the next line.

Copy link
Member Author

Choose a reason for hiding this comment

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

Other, less lossy ways to shorten the text may be:

Copy link
Member

Choose a reason for hiding this comment

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

ACK on shorten the text.

Copy link
Contributor

Choose a reason for hiding this comment

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

ACK on keeping the full title but removing "pull request".

@laanwj laanwj force-pushed the 2016_01_python_github_merge branch from b86484c to 04bdb61 Compare January 20, 2016 10:38
@laanwj
Copy link
Member Author

laanwj commented Jan 20, 2016

show the head SHA256 hash from the PR (possible short compare against the one you have ACKed)

The simplest idea there would be to print the commit message for the merge to the console. It has all context information: the pull #, the pull title, and the SHA (not 256) hashes for the commits in topological order.
(this can be done in a later pull, not going to do so now)

This is meant to be a direct translation of the bash script,
with the difference that it retrieves the PR title from github,
thus creating pull messages like:

    Merge bitcoin#12345: Expose transaction temperature over RPC
@laanwj laanwj force-pushed the 2016_01_python_github_merge branch from 04bdb61 to da6d18b Compare January 20, 2016 12:02
Copy link
Member Author

Choose a reason for hiding this comment

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

BTW: is anyone opposed to combining 'accept' and 'sign off' into one step? I may be overlooking something, but I've never completely understood why these are separate, why you would accept a change but then not sign it (which is mandatory).

Copy link
Member

Choose a reason for hiding this comment

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

They were separate at a time when the signing was not mandatory :)

@maflcko
Copy link
Member

maflcko commented Jan 20, 2016

Tested ACK da6d18b

@laanwj laanwj merged commit da6d18b into bitcoin:master Jan 20, 2016
laanwj added a commit that referenced this pull request Jan 20, 2016
da6d18b devtools: replace github-merge with python version (Wladimir J. van der Laan)
codablock pushed a commit to codablock/dash that referenced this pull request Dec 9, 2017
da6d18b devtools: replace github-merge with python version (Wladimir J. van der Laan)
@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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants