[scikit-learn] The culture of commit squashing

[Sebastian Raschka]

Oh wow, that looks like a neat feature, didn’t know about this, thanks for sharing! 
(And I would be in favor of this)

> On Jun 14, 2016, at 5:34 AM, Tom DLT <tom.duprelatour at orange.fr> wrote:
> 
> We could stop squashing during development, and use the new Squash-and-Merge button on GitHub.
> What do you think?
> Tom
> 
> 
> 2016-06-14 8:06 GMT+02:00 Matthieu Brucher <matthieu.brucher at gmail.com>:
> I don't even think that squashing them before the merge is actually sound. You will still need the history of why something happened several years down the road (and rebasing actually has a similar issue). This bit me quite often (having just one big commit to analyze after a merge from ancient VCS). Git was created to keep the history when merging, why would we explicitly remove knowledge?
> Just my 2 cents.
> 
> 2016-06-14 4:40 GMT+02:00 Jacob Schreiber <jmschreiber91 at gmail.com>:
> My research work involves frequently contributing small changes. I like to keep these around as a record of what I've done, until I've finished with that part of the code. However, I also hate having large numbers of commits (frequently can commit 50+ times a day without much substantitve progress). To combine these, usually I will avoid squashing commits in a branch until right before I merge it. This way you can review everything which has been done until you're finished with that branch, but also avoid having a large number of trivial commits. In this case, only after you've been given MRG +2 would you squash the PR. That would have a negative side effect of preventing the second reviewer from quickly merging the branch, though. 
> 
> What are your thoughts on that, Joel?
> 
> On Mon, Jun 13, 2016 at 6: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
> 
> 
> 
> 
> -- 
> Information System Engineer, Ph.D.
> Blog: http://blog.audio-tk.com/
> LinkedIn: http://www.linkedin.com/in/matthieubrucher
> 
> _______________________________________________
> 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