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.noth...@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@python.org > https://mail.python.org/mailman/listinfo/scikit-learn _______________________________________________ scikit-learn mailing list scikit-learn@python.org https://mail.python.org/mailman/listinfo/scikit-learn
