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