[scikit-learn] The culture of commit squashing

[Joel Nothman]

My concern is that people are responding to being asked to squash on one PR
by squashing during development on the next (as if merge were always
imminent). I want that to stop. Is part of the solution to stop squashing,
or make the person merging always perform the squash?

On 14 June 2016 at 12:53, Sebastian Raschka <mail at sebastianraschka.com>
wrote:

> Hi, Joel,
> in my opinion, it really depends on the particular case, but in general I
> am pro squashing — that is, when it happens at the very end. I also agree
> that squashing and force pushing while there’s still a review going on is
> clearly disruptive.
> Say there’s a new estimator being added. This often comes with an insane
> number of pull requests.
>
> - first take on EstimatorX
> - fix xyz in EstimatorX
> - address test case issue x
> - add additional test case to test edge case x
> - add code example for estimator x
> - fix typo in documentation for estimator x
> …
>
> So, once everything looks fine to the reviewers, everyone gave their okay,
> the CI tests pass, I think there’s nothing against summarizing it to a
> single commit:
>
> - implement EstimatorX
>
> In my opinion, it helps tracking down code in the commit history in the
> long run, but that’s just my personal opinion.
>
> Best,
> Sebastian
>
>
> > On Jun 13, 2016, at 9:36 PM, Joel Nothman <joel.nothman at gmail.com>
> wrote:
> >
> > For the last few years, there's been a notion that we should squash PRs
> down to a single commit before merging. Squashing can give a cleaner commit
> history, and avoid overrepresentation of minor work given silly commit
> count metrics used by Github and others. I'm not sure if there are other
> motivations.
> >
> > Recently I've seen several contributors amending commits and
> force-pushing changes. I find this disruptive to the reviewing process in a
> number of ways (links are broken; what's changed is hard to discern, when
> it could have been indicated in a commit message and diff; etc.). I have
> had to ask these several users to cease and desist.
> >
> > I also find that performing the squash can be unnecessary overhead
> either for the merger or the PR developer.
> >
> > I think squashing is more trouble than it's worth, except where:
> > * there are embarrassingly many minor commits in a PR
> > * squashing first makes a rebase easier because of concurrent changes to
> the codebase
> > * otherwise for cosmetic reasons only when there is low reviewing
> activity on the PR
> >
> > While squashing is far from the slowest part of our review process,
> being able to hit the merge button and move on would be great.
> >
> > Do others agree that a culture of amending commits in the ordinary case
> is counterproductive?
> >
> > (And apologies for wasting your time on such a silly issue, but I'm sick
> of clicking links in emails to find the commit's disappeared.)
> > _______________________________________________
> > scikit-learn mailing list
> > scikit-learn at python.org
> > https://mail.python.org/mailman/listinfo/scikit-learn
>
> _______________________________________________
> scikit-learn mailing list
> scikit-learn at python.org
> https://mail.python.org/mailman/listinfo/scikit-learn
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/scikit-learn/attachments/20160614/451f56a9/attachment.html>