BLD: update merge script to update on github #15917

jorisvandenbossche · 2017-04-06T08:37:15Z

@jreback @TomAugspurger This adds the ability to checkout a PR, do a small fixup or rebase, and push changes to github.
I now added it as an additional question whether to update or merge, but can also make to separate scripts of it for convenience.

codecov · 2017-04-06T09:29:17Z

Codecov Report

Merging #15917 into master will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #15917      +/-   ##
==========================================
+ Coverage   90.96%   90.99%   +0.03%     
==========================================
  Files         145      145              
  Lines       49534    49521      -13     
==========================================
+ Hits        45057    45062       +5     
+ Misses       4477     4459      -18

Flag	Coverage Δ
#multiple	`88.76% <ø> (+0.03%)`	⬆️
#single	`40.61% <ø> (-0.02%)`	⬇️

Impacted Files	Coverage Δ
pandas/types/common.py	`93.64% <0%> (-0.3%)`	⬇️
pandas/core/series.py	`94.89% <0%> (-0.08%)`	⬇️
pandas/indexes/base.py	`96.09% <0%> (-0.06%)`	⬇️
pandas/core/reshape.py	`99.27% <0%> (-0.01%)`	⬇️
pandas/core/frame.py	`97.57% <0%> (ø)`	⬆️
pandas/io/pytables.py	`93.06% <0%> (ø)`	⬆️
pandas/core/sorting.py	`97.81% <0%> (+0.03%)`	⬆️
pandas/types/dtypes.py	`95.38% <0%> (+0.04%)`	⬆️
pandas/indexes/multi.py	`96.7% <0%> (+0.1%)`	⬆️
pandas/io/parsers.py	`95.65% <0%> (+0.12%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1fbdc23...a06d0fc. Read the comment docs.

TomAugspurger · 2017-04-06T11:18:33Z

Just to make sure, the goal of this is to allow you to push your local changes back up to the PR branch, correct? And then use the green "Squash and merge" from there?

IIUC the new options are

Update the PR (y)
Merge the PR (maybe with local modifications (n)).

Maybe the logic should be flipped on the if / else you added, to be more consistent with how it was (y = merge), but not a big deal for me.

TomAugspurger · 2017-04-06T11:39:29Z

Need to remember to update https://github.com/pandas-dev/pandas/wiki/Merging-Commits

Also should have have a section there on commit message format (which I messed up with b070d51). Can link to http://pandas.pydata.org/pandas-docs/stable/contributing.html#committing-your-code

jorisvandenbossche · 2017-04-06T11:45:17Z

Just to make sure, the goal of this is to allow you to push your local changes back up to the PR branch, correct?

Yes, indeed, that's what I have used it for. I would prefer if we use the github green button more (but we can talk about in the meeting)

For that reason I put the update as 'yes' :), but can also switch or make it a different command ('update-pr.py')

jorisvandenbossche · 2017-04-06T11:45:45Z

Also should have have a section there on commit message format (which I messed up with b070d51).

What is wrong with it?

TomAugspurger · 2017-04-06T12:02:06Z

What is wrong with it?

Forgot the DOC code in the commit message title. Pretty minor :)

jreback · 2017-04-06T13:27:59Z

scripts/merge-pr.py


 merged_refs = [target_ref]

-merge_hash = merge_pr(pr_num, target_ref)
+update = continue_maybe2("Update PR and push to github (y) or merge locally (n)?")


let's make this explicity, maybe r (remote) or l (local) or q/n to do nothing.

any thoughts on this?

that's good, will upate

jorisvandenbossche · 2017-04-07T16:38:32Z

@jreback What you mentioned on the hangout about the commit message: as far as I can see, you can edit both commit title and longer commit message. What else do you want to edit?

TomAugspurger · 2017-04-07T16:39:12Z

scripts/merge-pr.py

+if update == 'r':
+    merge_hash = update_pr(pr_num, user_login, base_ref)
+elif update == 'l':
+    merge_hash = merge_pr(pr_num, target_ref)


Missing an else block? or an elif update == 'n'?

Oh nevermind, that's do nothing :)

TomAugspurger · 2017-04-07T16:44:26Z

I just did a test, and it seems like you can edit both the title and the squashed commit message when using the squash and merge button.

…

On Fri, Apr 7, 2017 at 11:38 AM, Joris Van den Bossche < ***@***.***> wrote: @jreback <https://github.com/jreback> What you mentioned on the hangout about the commit message: as far as I can see, you can edit both commit title and longer commit message. What else do you want to edit? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15917 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHIisNM3wLJXuiOog0nMzFmTTCQPlvks5rtmaLgaJpZM4M1SBd> .

jreback · 2017-04-07T16:45:55Z

so the script captures the top-of-the PR, which I usually want some of it because its a nice sentence or 2 on what is going on this PR. The button only includes the title.

BUT since we have https://github.com/pandas-dev/pandas/blob/master/.github/PULL_REQUEST_TEMPLATE.md, I have to edit this out most of the time (so maybe we should just remove this entirely).

jreback · 2017-04-07T16:46:48Z

here's what you get if you don't explicity edit things with the scripts. Not picking on you @TomAugspurger I accidently do this too :>

commit ba30e3a2e376035549b009079d44ba5ca7a4c48f
Author: alexandercbooth <[email protected]>
Date:   Wed Apr 5 16:47:07 2017 -0500

    BUG: addresses #14855 by fixing color kwarg conflict
    
     - [x] closes #14855    - [x] tests passed   - [x] passes ``git diff
    upstream/master | flake8 --diff``
    
    Author: alexandercbooth <[email protected]>
    
    This patch had conflicts when merged, resolved by
    Committer: Tom Augspurger <[email protected]>
    
    Closes #14871 from alexandercbooth/fix-color-scatterm-bug and squashes the following commits:
    
    3245f09b9 [alexandercbooth] DOC: moving whatsnew entry to 0.20.0
    8ff5f51f1 [alexandercbooth] BUG: addresses #14855 by fixing color kwarg conflict

jorisvandenbossche · 2017-04-07T16:50:03Z

I understand that copying from the first post into the commit message box is some extra work, but if you typically still need to edit the generated commit message by the script, it does not seem more work?

jreback · 2017-04-07T16:52:21Z

I understand that copying from the first post into the commit message box is some extra work, but if you typically still need to edit the generated commit message by the script, it does not seem more work?

what are you suggesting?

I think editing the commit messages if using the script is fine. just noting it.

I use the script because I often need to edit very minor things (like the formatting or something) in the whatsnew. Its just easier to do this right before merge.

jorisvandenbossche · 2017-04-07T17:00:20Z

what are you suggesting?

What I said on the meeting: using the merge button. You can still use the script to do minor things (that's the reason I the update part), but instead of merging locally and pushing, pushing to github and directly merging there (which means editing the commit message on github instead of locally the generated one -> and that is what I meant with not much difference in work as both typically needs editing)

jreback · 2017-04-07T17:06:16Z

@jorisvandenbossche ahh yes I see what you mean now.

ok, then I agree the default should actually be to push back to the user PR with any changes. (default is to do nothing I think).

jreback · 2017-04-07T18:58:29Z

thanks @jorisvandenbossche let me give this a try!

BLD: update merge script to update on github

f621c5e

jreback reviewed Apr 6, 2017

View reviewed changes

jreback added the Build Library building on various platforms label Apr 6, 2017

adapt question

a06d0fc

TomAugspurger reviewed Apr 7, 2017

View reviewed changes

jreback merged commit 88bed54 into pandas-dev:master Apr 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BLD: update merge script to update on github #15917

BLD: update merge script to update on github #15917

jorisvandenbossche commented Apr 6, 2017

codecov bot commented Apr 6, 2017 •

edited

Loading

TomAugspurger commented Apr 6, 2017

TomAugspurger commented Apr 6, 2017

jorisvandenbossche commented Apr 6, 2017

jorisvandenbossche commented Apr 6, 2017

TomAugspurger commented Apr 6, 2017

jreback Apr 6, 2017

jreback Apr 7, 2017

jorisvandenbossche Apr 7, 2017

jorisvandenbossche commented Apr 7, 2017

TomAugspurger Apr 7, 2017

TomAugspurger Apr 7, 2017

TomAugspurger commented Apr 7, 2017 via email

jreback commented Apr 7, 2017

jreback commented Apr 7, 2017

jorisvandenbossche commented Apr 7, 2017

jreback commented Apr 7, 2017

jorisvandenbossche commented Apr 7, 2017

jreback commented Apr 7, 2017

jreback commented Apr 7, 2017

BLD: update merge script to update on github #15917

BLD: update merge script to update on github #15917

Conversation

jorisvandenbossche commented Apr 6, 2017

codecov bot commented Apr 6, 2017 • edited Loading

Codecov Report

TomAugspurger commented Apr 6, 2017

TomAugspurger commented Apr 6, 2017

jorisvandenbossche commented Apr 6, 2017

jorisvandenbossche commented Apr 6, 2017

TomAugspurger commented Apr 6, 2017

jreback Apr 6, 2017

Choose a reason for hiding this comment

jreback Apr 7, 2017

Choose a reason for hiding this comment

jorisvandenbossche Apr 7, 2017

Choose a reason for hiding this comment

jorisvandenbossche commented Apr 7, 2017

TomAugspurger Apr 7, 2017

Choose a reason for hiding this comment

TomAugspurger Apr 7, 2017

Choose a reason for hiding this comment

TomAugspurger commented Apr 7, 2017 via email

jreback commented Apr 7, 2017

jreback commented Apr 7, 2017

jorisvandenbossche commented Apr 7, 2017

jreback commented Apr 7, 2017

jorisvandenbossche commented Apr 7, 2017

jreback commented Apr 7, 2017

jreback commented Apr 7, 2017

codecov bot commented Apr 6, 2017 •

edited

Loading