Squash and merge

[Hainish]

@J0WI I think we should squash and merge by default, especially for ruleset changes. This will make the git history a bit cleaner, especially since PRs are tending to add additional commits just to get the tests to re-run.

https://help.github.com/articles/about-pull-request-merge-squashing/

[Hainish]

cc @fuglede

[luixxiul]

like this: #5788

[Hainish]

Ah right, so it looks like this is already being done for the most part.

[luixxiul]

Hi, I usually squash my commits by myself locally before/after I create a PR. On #5788 commits are not squashed at all

[Hainish]

I think it's fine if contribs don't squash their PRs, as long as the maintainers do it upon merge. The end result is the same, but with our range of contributors it's more difficult to enforce on their end.

[luixxiul]

@Hainish the reason why I do locally is that I tend to forget to pull the squashed commit before making any change on the local repo and mess it up quite badly. It is OK if you do not forget to do that.

[fuglede]

Noted; I've typically asked contributors to do it themselves for long-winded PRs with more than, say, 3-4 trivial commits, but I'm fine with just doing it upon inclusion. Any hard limits for what is acceptable?

[Hainish]

@fuglede no universal rule can be made here. If greater granularity is required, it may make sense to break it out into more than a few commits. But I mostly envision this happening for features and code changes, and less for ruleset changes. I could see a PR that, say, included multiple types of sites across a few different rulesets having multiple commits that may make sense. But in that case it's probably more appropriate to have those changes in multiple PRs and not just one.

[J0WI]

I like to have unsquashed commits in PR to see what changed since the last feedback.
Squash before merge seems good to me and I'll do this in future.

[Hainish]

Thanks!

[fuglede]

Could you activate the GitHub feature to make it an option to do this automatically upon merging for the repo? I never tried it but it looks like it will make the process quite a bit easier.

[Hainish]

It looks like in the admin UI, I don't have the option to make it default. I only have the option to uncheck either of the merging methods, which we don't want to do.