phpro / grumphp

A PHP code-quality tool
MIT License
4.15k stars 431 forks source link

GrumPHP shouldn't validate merge commits #308

Closed Bilge closed 7 years ago

Bilge commented 7 years ago
Q A
Version 0.11.1
Bug? yes
New feature? no
Question? no
Documentation? no
Related tickets n/a

GrumPHP currently validates all commit types. However, it seems inappropriate to validate a merge commit since it brings in many unrelated changes in files the author did not modify. I think we should detect merge commits and skip them by default. Perhaps we can add an option to include them but I currently do not see a use case for this.

This might help...

Kanti commented 7 years ago

GrumPHP should only skip merge commits if there was no conflict.

Bilge commented 7 years ago

@Kanti Why? A merge commit brings in other people's work for which you are not responsible.

veewee commented 7 years ago

You might just want to use the git_conflict task and rebase your local changes against the upstream branch before a commit. When you are actually merging branches, you do want to know what is going wrong and fix it. If somebody else caused the error, you can still tell hem/her to go fix it.

For me that is more then enough.

Bilge commented 7 years ago

My company frowns upon rebases because it rewrites history. If there are already comments and/or reviews on the PR they become invalidated by the rebase. In such cases a merge is preferred, and in such cases, we do not want GrumPHP to validate the merge commit.

However, it seems such a feature is divisive so it is clear that if we are going to implement it, it should be optional.

veewee commented 7 years ago

Even though it is a merge commit, GrumPHP should still return green. If it doesn't, there is something wrong with your codebase. In my opinion, it is not a good idea to commit broken code on a shared branch. You might want to review your git flow so that this is not possible.

I really don't think this is a good addition to this project: it's adding complexity without any reasonable added value.