[Pandas-dev] Thoughts on adopting a 1-PR-1-commit policy?

Mon Jun 6 09:58:52 EDT 2016

I c, ok then that seems reasonable for small ones.

Occasionally I have to fix a PR, so i pull it down, fix and then merge. It
ends up looking very similar. Again these
are usually very small / limited changes, but author just doesn't fix
things.

I usually have to manually add the issue reference in that case as the PR
header text is not included.

But that's why I review the commit message anyhow.

So ok by me.

Jeff

On Mon, Jun 6, 2016 at 9:46 AM, Joris Van den Bossche <
jorisvandenbossche at gmail.com> wrote:

> I tried out the button yesterday to see what it does with a small patch:
> https://github.com/pydata/pandas/pull/13369 (and the actual commit:
> https://github.com/pydata/pandas/commit/e90d411714e7deac73e3e6b763ba9dccd3549871
> ).
>
> So you can see I also edited the commit message (as the PR title didn't
> reflect the actual change anymore).
> So in general the button can capture a similar workflow (certainly for
> small patches). One difference is that you do not get links to the
> individual commits (only a list of the messages), but I am not sure this is
> essential (you can also see those in the PR).
>
> Joris
>
> 2016-06-06 15:25 GMT+02:00 Jeff Reback <jeffreback at gmail.com>:
>
>> This IS currently enabled (merge squashing only). Though I haven't
>> clicked the button so not exactly sure what it actually outputs. So I
>> suppose it gives basically the same output then it would be ok.
>>
>> Using the script, I generally edit the commit to remove the
>>
>>  - [ ] closes #xxxx
>>  - [ ] tests added / passed
>>  - [ ] passes ``git diff upstream/master | flake8 --diff``
>>  - [ ] whatsnew entry
>>
>> which is what our PR template gives (and unless the user removes it it
>> will propogate to the log)
>>
>> I *mostly* make them look like this, which is concise, yet retains all of
>> the pertinent info.
>>
>> commit 6edf4471d1e55ce6b587a7e37dd540d787716413
>> Author: Mike Graham <mikegraham at gmail.com>
>> Date:   Mon Jun 6 08:10:10 2016 -0400
>>
>>     DOC: Fix wording/grammar for rolling's win_type argument.
>>
>>     Author: Mike Graham <mikegraham at gmail.com>
>>
>>     Closes #13376 from mikegraham/master and squashes the following
>> commits:
>>
>>     ec0c88e [Mike Graham] DOC: Fix wording/grammar for rolling's win_type
>> argument.
>>
>> So all for using the button if it can capture a similar workflow (just
>> haven't tried it out).
>>
>> The biggest reason NOT to use the button is that it gives you a chance to
>> do a final check / test on your local machine with the actual merged
>> content. More that a few times I have found issues and backed out of the
>> process. Though for simpler changes (like a doc-typo that's not necessary).
>>
>> Finally merging locally DOES allow me to edit the whatsnew for
>> consistency and such (or other files). Generally these are trivial things
>> like spacing / doc-style and such, where a back-and-forth is not necessary.
>>
>> Jeff
>>
>>
>> On Mon, Jun 6, 2016 at 8:56 AM, Joris Van den Bossche <
>> jorisvandenbossche at gmail.com> wrote:
>>
>>> Triggered by a recent tweet of Wes (
>>> https://twitter.com/wesmckinn/status/738111494852775936), I thought it
>>> would maybe a good time to evaluate the change in merging practices in
>>> pandas.
>>> I am certainly positive on the change in general (my only concern is
>>> that we shouldn't do additional changes in the middle of using the merge
>>> script, so you can still look at the 'Files changed' on github to see what
>>> actually changed).
>>>
>>> But, the main thing I was wondering: in the meantime github added the
>>> feature to squash on merge through the green merge button (
>>> https://github.com/blog/2141-squash-your-commits). Are there still
>>> advantages of using our own custom script over the green button?
>>>
>>> - As far as I see, the main difference is that the github button does
>>> not include the text of the first comment in the squashed commit (but
>>> certainly for smaller PRs, this is not really a problem IMO, and the commit
>>> message gets a link to the PR)
>>> - On the plus side, it is easier (certainly for quickly merging a small
>>> PR, using the merge script is some overhead IMO), and you get the visual
>>> indication on github that the PR is 'merged' instead of 'closed' (but
>>> actually merged).
>>>
>>> Jeff, would you be OK with starting to use the green button again (with
>>> the squash on merge option, but you already disabled the merge commit
>>> option), at least for the smaller PRs?
>>>
>>> Regards,
>>> Joris
>>>
>>>
>>> 2016-01-18 23:59 GMT+01:00 Wes McKinney <wesmckinn at gmail.com>:
>>>
>>>> On Sun, Jan 17, 2016 at 3:34 PM, Wes McKinney <wesmckinn at gmail.com>
>>>> wrote:
>>>> > Yeah, the point of this issue is not "there shall be no more than 1
>>>> > commit per PR" but rather that smaller patches (i.e., most patches)
>>>> > should not degrade the signal-to-noise ratio of our commit history.
>>>> > Further, we should avoid merging commits that don't stand on their
>>>> > own. Lastly, merge commits generally only serve to degrade the SnR
>>>> > ratio.
>>>> >
>>>> > Let's look at a sample of yesterday's commits:
>>>> >
>>>> >
>>>> https://www.dropbox.com/s/mp5yfp76h6h8z3y/commit-log-20160116.png?dl=0
>>>> >
>>>> > No mistakes were made here, except that our current process (which
>>>> > Jeff has been following diligently) is resulting in a commit history
>>>> > that is less useful than it could be.
>>>> >
>>>> > My preference is:
>>>> >
>>>> > - Use the patch tool for smaller patches and large patches that
>>>> > haven't been split out into a series of incremental, standalone
>>>> > patches
>>>> > - For large patches that make sense as multiple incremental commits,
>>>> > none of which breaks the build, merge with --ff-only (rebasing as
>>>> > needed). I expect this to be rare.
>>>> >
>>>> > I really like to avoid "edge-case driven development" -- we are bound
>>>> > to have patches where this guidance doesn't feel right, and we
>>>> > definitely don't have to dogmatically follow it.
>>>> >
>>>>
>>>> For the record -- I spent some time reviewing the major category dtype
>>>> pull requests that were merged in 2014, and given the sprawling nature
>>>> of those changes and the huge amount of collaboration that took place,
>>>> I agree it would have been preferable to fast-forward merge the
>>>> incremental commits instead of squashing them into a couple of
>>>> monolithic commits.
>>>>
>>>>
>>>> https://github.com/pydata/pandas/commit/0f62d3fc62f317538044ed3d349bfb89fb7ee9de
>>>>
>>>> https://github.com/pydata/pandas/commit/ea0a13c172761348d08285a19ebf731cdabb2db3
>>>>
>>>> > - Wes
>>>>
>>>
>>> _______________________________________________
>>> Pandas-dev mailing list
>>> Pandas-dev at python.org
>>> https://mail.python.org/mailman/listinfo/pandas-dev
>>>
>>>
>>
>
> _______________________________________________
> Pandas-dev mailing list
> Pandas-dev at python.org
> https://mail.python.org/mailman/listinfo/pandas-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/pandas-dev/attachments/20160606/b577c63f/attachment-0001.html>