Over the last 3 years, I’ve reviewed more than 1,000 pull (merge) requests. During that time I learned a lot – mostly about how not to review code,...
For further actions, you may consider blocking this person and/or reporting abuse
Very good points, I like the "I ran this locally" - might be adding that flag :)
Turns out I don't also have to be a QA tester when I'm approving pull requests... this is a great idea.
It's a life-saver :D
I think it's OK to be direct about problems and suggest specific improvements, provided you phrase them right. For example "this variable name violates our coding standards [link] because [reason]. How about $wibbly_boing instead?" (but not "I see you haven't read our coding standards, this should be $wibbly_boing"). You should also say stuff like "this is great, I wouldn't have thought of doing it that way, hey @everyone take a look" sometimes.
When I'm writing documentation on working practices I put it something like this: "Code review is not a competitive sport with winners and losers, it is a never-ending conversation aimed both at making improvements but also pointing out best practice.".
As for "I ran this locally" I usually won't even bother reviewing until the automated tests have passed, which makes most of the problem go away. There's still the issue of non-existent (or flawed) tests which mean the code still hasn't been run locally, but that's the sort of thing I'd expect code review to catch. One of the automated checks I like to see (although most places don't have it) is that the test coverage is the same as or higher than the coverage of the merge target. That is again just a guideline, but a dev can expect Questions if he makes the coverage go down!
From Linus Torvalds <>
Date Fri, 6 Jul 2012 13:30:01 -0700
Subject Re: Regression - /proc/kmsg does not (always) block for 1-byte reads
Kay, this needs to be fixed.
Suggested fix: just use the 'seq_printf()' interfaces, which do the
proper buffering, and allow any size reads of various packetized data.
Of course, I'd also suggest that whoever was the genius who thought it
was a good idea to read things ONE F☠ BYTE AT A TIME with system
calls for each byte should be retroactively aborted. Who the f☠ does
idiotic things like that? How did they noty die as babies, considering
that they were likely too stupid to find a tit to suck on?
Linus
😇
Hahahahaha we can let him slide I guess
from my point of view, you should do this only when you have question to PR or if it is quite big, otherwise it is just waste of time.
i don't see how 'sitting with author to review code' it can help both grow.
this looks really great
I completely agree that if a MR od straight forward, there is actually no need to sit together. Will update to make it explicit.
For the growth effect, it's more relevant to learn how to better communicate, teach and take real-life criticism.
Voice feedbacks is always great :) But of course, as mentioned the PR should be "important" (yeah, it's relative... But if the PR only changes a very small thing it's not necessary).
I'd also add something to improve the conversation: I always read the PR before (maybe two times) & try to understand briefly, if I can't I write a question. In brief, I prepare the meeting.
I had a tech-lead that didn't read any PR before the sit, and so he was reading the code along with me. It was such a pain and a wast of time... That's why I use this process for me to review PR :)
Nice post :-) One thing I changed in my code review style - In the past, I always reviewed the code just in e.g. GitHub, but I soon switched to actually checking out the code locally and having a bit more of an explore. Found that I would spot things in my IDE that I never would have flagged just by looking on GitHub. Seems so obvious really but it really made a big difference!
I can confirm from my experience that those are really important points! Thanks for the article.
A have a concern to add to the point "Sit with the author to review the code". I need some time alone for every PR. With just few exceptions. I need some time to go over the code line by line, mark conspicuous parts, and check them. This may take more than 30 minutes for many pull requests because I really make sure to check everything. I already found many important bugs like this. If the author is sitting next to me for the whole time, it may be boring for him/her. And I feel under time pressure.
Nevertheless, I find this is a good point and I will try to sit more together with the author.
Thanks for your input. I agree that you need to have deep concentration to fully review code
Thanks for this post, it highlights things that are very important and yet often remain unspoken.
If your org is doing code reviews, it's worth writing something like that down and referring everyone to it.
It will not only push reviewers to be considerate (as is recommended here) but it will also make people who send PRs feel a lot more comfortable. You gotta admit that sending your first PR can be pretty intimidating sometimes, that pressure needs to go.
I am really up for the "I ran this locally" concept. Remember when I had to implement this for a dev team by running all unit tests automatically and update a file before completing a successful git push command when triggered.
"Bigger requests require exponentially more time to review." I disagree with this. It really boils down to how complex the changes are, but most of the time it's much easier to review 1 PR that's 20 files than 2 PR's that are 10 files each. Each PR takes some setup time to get the code running and understand what changes are happening. And even if the bigger PR required more time, it wouldn't be "exponentially" more.
Hmmmm maybe we're looking at it differently. It seems to me that you are talking about having one logical change (e.g. feature) in 1 MR vs. 2 MRs. I agree that 1 MR is preferable.
But I often see someone develop a feature and do a refactor or an unrelated bug fix, contributing to the file changed count. In that case I would rather have 2 MRs than one, because I'm losing focus from the important things.
These are great tips. I like the template that everyone uses when creating a pull request! 👍🏻
And still there are pull requests with just a title. 🤦♂️
In an ideal world the PR would automatically tested on a production like environment so there should be no need to run the code locally.
Great post! So many useful insights
Should one split a feature into multiple PRs to make them feasible for review? Like PR per task within the feature scope.
Ideally, the feature would be small enough to make for a reasonable PR. Breaking up the PR would split the context - it's hard to see the big picture.
For example, let's say that you're developing a web app and you are adding comments section. Maybe it would make sense to split the PR into backend and frontend, especially if those two are reviewed by different people.
Thanks for reply! Sure, feature slicing is always a good idea but I am talking about situations when feature cannot be further sliced down.
Yeah, then it's better to have it as one PR. What helps me the most then is to sit with the author and speak my thoughts out loud. It makes the process go faster
Nice points!
Small typo found: ... we all actually knew that it should be ...
Updated :)
I love it!
Super useful suggestions. Thought you'd want to know about a small typo:
"we all actually new that it should be done."
I think should be knew as in
"we all actually knew that it should be done."
Adding a "reviewer(s) ran the code locally" checkbox would be a good idea as well. 8}