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

Mon Jun 6 09:46:50 EDT 2016

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
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/pandas-dev/attachments/20160606/667f0df7/attachment.html>