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

devtools: replace github-merge with python version #7378

Merged
merged 1 commit into from Jan 20, 2016

Conversation

Projects
None yet
4 participants
@laanwj
Member

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]

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jan 19, 2016

Member

Nice!
Tested ACK (on a different repo: digitalbitbox/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?)
Member

jonasschnelli commented Jan 19, 2016

Nice!
Tested ACK (on a different repo: digitalbitbox/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?)
@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jan 19, 2016

Member

utACK b86484c

Member

MarcoFalke commented Jan 19, 2016

utACK b86484c

req = urllib2.Request("https://api.github.com/repos/"+repo+"/pulls/"+pull)
result = urllib2.urlopen(req)
result = json.load(result)
return result['title']

This comment has been minimized.

@MarcoFalke

MarcoFalke Jan 19, 2016

Member

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: "]

@MarcoFalke

MarcoFalke Jan 19, 2016

Member

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: "]

This comment has been minimized.

@laanwj

laanwj Jan 20, 2016

Member

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.

@laanwj

laanwj Jan 20, 2016

Member

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.

This comment has been minimized.

@laanwj

laanwj Jan 20, 2016

Member

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

  • leave out the "pull request" completely, as in "Merge #1234: bla"
  • abbreviate it as PR
    ...
@laanwj

laanwj Jan 20, 2016

Member

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

  • leave out the "pull request" completely, as in "Merge #1234: bla"
  • abbreviate it as PR
    ...

This comment has been minimized.

@MarcoFalke

MarcoFalke Jan 20, 2016

Member

ACK on shorten the text.

@MarcoFalke

MarcoFalke Jan 20, 2016

Member

ACK on shorten the text.

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 20, 2016

Member

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

@jonasschnelli

jonasschnelli Jan 20, 2016

Member

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jan 20, 2016

Member

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)

Member

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)

devtools: replace github-merge with python version
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 #12345: Expose transaction temperature over RPC
if os.path.isfile('/etc/debian_version'): # Show pull number on Debian default prompt
os.putenv('debian_chroot',pull)
subprocess.call([BASH,'-i'])
reply = ask_prompt("Type 'm' to accept the merge.")

This comment has been minimized.

@laanwj

laanwj Jan 20, 2016

Member

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).

@laanwj

laanwj Jan 20, 2016

Member

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).

This comment has been minimized.

@sipa

sipa Apr 3, 2016

Member

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

@sipa

sipa Apr 3, 2016

Member

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

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jan 20, 2016

Member

Tested ACK da6d18b

Member

MarcoFalke commented Jan 20, 2016

Tested ACK da6d18b

@laanwj laanwj merged commit da6d18b into bitcoin:master Jan 20, 2016

1 check passed

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

laanwj added a commit that referenced this pull request Jan 20, 2016

Merge pull request #7378
da6d18b devtools: replace github-merge with python version (Wladimir J. van der Laan)

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge pull request bitcoin#7378
da6d18b devtools: replace github-merge with python version (Wladimir J. van der Laan)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge pull request bitcoin#7378
da6d18b devtools: replace github-merge with python version (Wladimir J. van der Laan)

codablock added a commit to codablock/dash that referenced this pull request Dec 9, 2017

Merge pull request bitcoin#7378
da6d18b devtools: replace github-merge with python version (Wladimir J. van der Laan)

codablock added a commit to codablock/dash that referenced this pull request Dec 9, 2017

Merge pull request bitcoin#7378
da6d18b devtools: replace github-merge with python version (Wladimir J. van der Laan)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment