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...
For further actions, you may consider blocking this person and/or reporting abuse
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
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.
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.
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.
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.
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.
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.
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.
Really valuable post!
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!
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
+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.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 diffto see what I changed and what I want to commit. If I'm happy with all unstaged changes being committed I usegit add ., but if I see "garbage", I simply usegit add -p .After that I do a
git diff --stagedand if nothing went wrong I do a simplegit commit -mwith a descriptive commit message. When I'm done I do agit checkout -- <file>to get rid of the garbage that I didn't commit, or a simplegit reset --hardif 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.
Nice ideas, Martin, thanks!
Great article Martin! Very good information!
This is all good advice, and I prefer to take it further by asking for reasons in the commits themselves.
Git is a Communication tool
Jesse Phillips
Great article Martín, thanks!
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.
I wanted to improve my PRs in my personal projects and this post was just perfect, easy to understand and very valuable
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.