[scikit-learn] The culture of commit squashing

[Sebastian Raschka]

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