node-wot
node-wot copied to clipboard
eclipse-thingweb
Change default merge strategy
On PR #519 @erossignon suggested changing the default merge strategy from "Allow merge commits" to "Allow rebase merging". Rebasing would allow having a more linear history in the master branch where the current approach is to have merge commits of different PRs. I work with both approaches in different repositories and I have to admit I like both of them.
My take is that the current approach becomes not useful when you have a lot of feature branches (a.k.a. PRs) in a short amount of time. I personally like the fact that you can understand from which PR a change was generated and use this information to look back in Github the discussion history. But of course, when you merge 20/30 Prs a week the thing is not really manageable. But this is not the case yet, looking at node-wot git graph I can still tell what's going on:
Please share you thoughts about this topic below :)
There is also this part in the Contributing.md about this : https://github.com/eclipse/thingweb.node-wot/blob/master/CONTRIBUTING.md#pull-requests-and-feature-branches
It is not exactly the same but some might be doing this while others are not. It would be good to say that rebase is required so that we have a single style of PRs? Not a strong opinion though :)
There is also this part in the Contributing.md about this : https://github.com/eclipse/thingweb.node-wot/blob/master/CONTRIBUTING.md#pull-requests-and-feature-branches
I didn't notice! 😱 then I think we should follow our own guidelines 🤣 and maybe add a PR template as a remainder to new contributors (and old ones) the good policies that we set.
There is also this part in the Contributing.md about this : https://github.com/eclipse/thingweb.node-wot/blob/master/CONTRIBUTING.md#pull-requests-and-feature-branches
I didn't notice! 😱 then I think we should follow our own guidelines 🤣 and maybe add a PR template as a remainder to new contributors (and old ones) the good policies that we set.
Not sure "who" created the guidelines. I guess it was @mkovatsc. Having said that, I think we need to agree now that it is still the way to to go. I don't have a strong opinion.
Looking at the commit history it is sometimes very confusing if one PR has a longer commit history. The more I think about the more I would like to change the default merge strategy from all commits to "squash and merge".
What do others think?
The guidelines combined what is used in other Eclipse projects and what we used for inner source.
A squash commit is good, when the PR is limited to a single feature or fix. A review before merging could ensure this. Then it could be as clean and modular as the rebase, and even easier to understand with a single commit.
No, I meant being more strict with PRs (that they only add a single feature / fix a single bug) when changing to squash and merge.
I totally agree with your statement. We tried to do that in the past.. and surely failed sometimes which gives room to improve.
Anyhow, even if a PR tackled just one aspect but was developed over several commits the history on master does not convey that. Hence, I prefer having a single commit that essentially says what was fixed/improved in a given PR and leads to s single entry in the history.
Note: projects like Java, JavaFX etc do that as well.
Anyhow, even if a PR tackled just one aspect but was developed over several commits the history on master does not convey that. Hence, I prefer having a single commit that essentially says what was fixed/improved in a given PR and leads to s single entry in the history.
I'm ok with squashing too, as long as the other commits in the PR are:
chores of package JSON related with the changes of the PR- Style fixes
- Reviews suggestions
I would not squash if:
- PR actually introduces more than on bugfix or feature
- Reviews ask to implement additional features