DEV Community

Discussion on: How to create effective Pull Requests

Collapse
 
jessekphillips profile image
Jesse Phillips

I disagree with squash on merge. I want the merge request to consist of atomic commits I can review.

Like the formatting requirement, I still think it needs more coordination, but if it is done in a different commit then the logic changes it being in the same merge request is not an issue.

If you touch a thousand file and say added argument documentation, that is still a quick review.

Collapse
 
mpermar profile image
Martín Pérez

Squash is valid when there is a lot of back and forth within the commits themselves which sometimes happens. I do A, commit, change my mind, then do B and commit, then change my mind again and do C and commit and finally have to capitulate and get back to approach A and commit. Time to send a PR and I end up with a complete commit history but probably a history that I should clean up before submitting a PR.

Thread Thread
 
jessekphillips profile image
Jesse Phillips

I'm not saying there aren't times to take advantage of of it, but I have concerns because tools provide that as policy and I don't want people to take the recommendation that way.

Thread Thread
 
smeijer profile image
Stephan Meijer

There are always exceptions to the rule. I think with squash the most important thing is to cleanup before you push. Squash what should be squashed but never squash with something that has already been pushed.

Especially not after a (first) review/change-request! When you're rewriting history, I need to do over my entire review. Instead of just checking the changes since last the time.

Thread Thread
 
jessekphillips profile image
Jesse Phillips

I like the, add commits after review point. I intend to make the merge request a work in progress at that point and rebase when the review is complete.

My comment on squash was because gitlab can make every merge request squash before merge automatically.