DEV Community

Cover image for How to create effective Pull Requests

How to create effective Pull Requests

Martín Pérez on April 09, 2019

Pull Requests, from now on referred to as PRs within this article. That unsexy but necessary step that sits between your wonderful shiny new commit...
Collapse
 
tonymet profile image
Tony Metzidis

I would add : rebase and squash your commits. This way when the PR is merged, the mainline is a clean series of atomic & significant commits

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.

Collapse
 
mpermar profile image
Martín Pérez

This is totally right and great advice, especially as PRs tend to hide on UIs when merged and closed but commits stay more accessible. Thanks, Tony.

Collapse
 
jeffblanco profile image
JeFFBlanco

Great Post!

I agree that formatting and whitespace tweaks add to the overall noise of the diff, but I also think there can be appropriate times and a places to get the code tidy. If the PR Author knows a file is unchanged they could add a "heads up" comment for the reviewer. Alternatively, they could open a separate PR that is purely formatting/code cleanup.

Collapse
 
ben profile image
Ben Halpern

Really valuable post!

Collapse
 
ty profile image
Ty • Edited

How would you recommend someone to "Avoid void changes" and "Avoid formatting changes"? I often see those on my PRs, but I'm not sure how to clean them up.

Thanks for the timely post by the way!

Collapse
 
mpermar profile image
Martín Pérez

You can always append corrective commits when this happens.

If this is rather a problem of different members using different setups ( e.g. tabs, spaces or spaces as tabs) then you should collectively agree on a common format for your favorite development environment, store and share it.

Or alternatively you can resort into something like prettier.io/ which integrates with many languages and tools

Collapse
 
smeijer profile image
Stephan Meijer

+1 for prettier.

Also; if you still using the cmd line, please start using a graphical git client instead. Gitkraken, sourcetree, tower, there are enough. With a graphical client it's so much more obvious what you are going to commit.

Unstage/discard all 'garbage' and only commit after reviewing the changes. No more blind git add . && git commit -m.

Thread Thread
 
serializator profile image
Julian • Edited

I think that it's not a matter of whether someone uses the CLI or a graphical client for this, but more the overall experience, mindset and workflow of that person.

I personally always use a git diff to see what I changed and what I want to commit. If I'm happy with all unstaged changes being committed I use git add ., but if I see "garbage", I simply use git add -p .

After that I do a git diff --staged and if nothing went wrong I do a simple git commit -m with a descriptive commit message. When I'm done I do a git checkout -- <file> to get rid of the garbage that I didn't commit, or a simple git reset --hard if I really don't wanna keep anything.

What I think is that you can be as careless with both the CLI as you can be with a graphical client, it depends on how much effort you want to put into keeping your Git history clean and organized.

Collapse
 
pavlosisaris profile image
Paul Isaris

Nice ideas, Martin, thanks!

Collapse
 
david_j_eddy profile image
David J Eddy

Great article Martin! Very good information!

Collapse
 
jessekphillips profile image
Jesse Phillips

This is all good advice, and I prefer to take it further by asking for reasons in the commits themselves.

Collapse
 
rhymes profile image
rhymes

Great article Martín, thanks!

Collapse
 
cstryker profile image
Colin Stryker

Couldn't agree more with the notion of avoiding huge PRs. I am a great believer in committing frequently. Huge PRs are a burden on your team. Sometimes they are unavoidable, but often you can find ways to introduce change in manageable chunks.

Collapse
 
ykbastidas profile image
Yorfrank Bastidas

I wanted to improve my PRs in my personal projects and this post was just perfect, easy to understand and very valuable

Collapse
 
mpermar profile image
Martín Pérez

Btw, edited and added a section about Single Responsibility Principle on PRs.

Just experienced one of those today which recalled me that it is actually quite important.