search

coapjs / node-coap

CoAP - Node.js style
MIT License
531 stars 154 forks source link

Change merge method for PRs to "Create a merge commit"? #318

Closed JKRhb closed 2 years ago

JKRhb commented 2 years ago

So far commits are always squashed when a PR is merged. To make the tracking of changes easier we might consider creating a merge commit instead. In this case, however, we might add the requirement of performing a rebase before a PR is merged to ensure a clean commit history.

What do you think? @Apollon77 @invaderb

Apollon77 commented 2 years ago

The question is if the "requirement to do a rebase directly before merge" makes sens e... Sure if you @JKRhb or me do that it is easy to organize ... but think about an "other developer" ... What speaks against the squash? It is nothing else like a "forced implecit rebase" ... the only info you loose are the deifferent commits.

In fact because at the end you or me will merge (at least for now) we can also open up and just "agree" on basic rules/idea on when to use a squash instead of a merge. no issue with that

JKRhb commented 2 years ago

@Apollon77 You made some good points. Maybe we could say if the commit history of an "external" PR is not very clean then we could just squash during merge. Otherwise, after a potential rebase, we create a merge commit?

Apollon77 commented 2 years ago

Agreed. Yes it is not "hard defined" but in my experince working best.

Apollon77 commented 2 years ago

But I think @mcollina needs to adjust that in the repo settings :-) ... if he also agreeed to this "convention"

invaderb commented 2 years ago

Honestly I've never been a fan of rebasing, but I don't a strong opinion to not do it just a personal thing.

mcollina commented 2 years ago

I prefer the squash than the rebasing.

JKRhb commented 2 years ago

Thank you for your input everyone :) I think we can conclude that we keep the current procedure.