Good Points. The most important consideration from my experience having worked with more than 40 teams: "Code review is not the time for you to impose your personal taste on a colleague." There are always many acceptable ways how to solve a problem in software development, so if someone imposes his personal preference over that from a colleague this creates tension and a time sink for the team - don't make this a blocking review! This also plays a big role when developers obsess over code but lack the vision and experience to see that not everything will be a relevant factor for technical debt in the grand scheme for the business.
I disagree. The point of code review (and indeed, software engineering) is to get at the difference between code that works for its intended purpose (as far its author knows) and code that it's reasonable for us to be responsible for collectively and in perpetuity. That includes things like working in the way you would expect, minimizing surprise, using names that make sense, etc. which are fundamentally subjective and human factors. It's expected that a new team member bounces off some "not the way I'd have done it" type comments for a while until they're socialized into the way that the team or project does things and can reproduce it themselves the first time. Formatters, linters, and documentation can speed up this acculturation process but there is an irreducible degree of "good taste" which you just have to cultivate over time.
That said, it's also important the reviewer feels equally responsible for getting the work done and is willing to make pragmatic tradeoffs taking into account timeline pressure, how contained something is, how likely it is to actually cause trouble, how hard it is to change later. Cross-team and cross-org reviews are more likely to involve pure gatekeeping, which is death.
The sort of clean that a functioning restaurant is, is something many people don't achieve at home.
I wonder sometimes if these arguments in software are less of an issue of lack of care for craft, and more just a fundamental misunderstanding that the sorts of arguments and compromises one makes with a roommate over what state the apartment is allowed to be in are simply not analogs for a professional setting.
I get the very distinct feeling sometimes that my mis en place rules are being received as if I'm whining about whether coasters get put away when not in use, and how long old magazines get left on the couch.
Nearly all of the fastidiousness I can manage in a day I save for work, so I'm very familiar with the other side of this conversation outside of the office.
Consistency and predictability could also be considered a sign of maturity. Maturity is something to be observed, not declared.
It also depends where you draw the line. For example, is dependency injection a matter of taste or a fundamental design decision for a project? What about "defensive" coding practices, like `0 == x` over `x == 0` in C?
What does 0 == x vs x == 0 have to do with defensive coding practices? I have always understood defensive coding practices to mean always check your pre-conditions.
In C "x = 0" is a legal expression, so you could have if(x = 0) { ... } and it would assign 0 to x and then treat it as false. The compiler won't complain (unless you have warnings turned on, which you should). But if you do "if (0 = x)", you're assigning to a constant, which will result in a compile error. So "if (0 == x)" guards against a typo where you omit the second =.
The GMail outage of 2010, where Google had to restore 10% of GMail accounts from tape backup, was precisely because of this. A C++ migration script had used the "x = 0" construct, which set the variable to false and made the script go haywire. So not a theoretical concern. The other major takeaway from this was to always treat your migration scripts like production code and use the same warnings, tests, and defensive programming that you would use everywhere.
I prefer 0 == x, because you can skim code faster and the first few words serve as a documentation. Compare:
if (TIMEOUT ...
if (NO_SETUP ...
if (REQUESTING
if (PENDING
if (DEACTIVATED
if (USER_INVALID ...
to:
if (object.action.send_timestamp ...
if (object.state == ...
if (object.state == ...
if (object.state == ...
if (object.state == ...
if (database.users_query (USER_BY_STATE, ...
In C it is a "defense" against inadvertent x=0 being assignment. The definitions are cloudy enough that you could consider it defensive programming or not.
This is one of the biggest traps I’ve seen in code review. Generally, everyone is coming from a good place of “I’m reviewing this code to maintain codebase quality. This technically could cause problems. Thus I’m obligated to mention it”. Since the line of “could cause problems (important enough to mention)” is subjective, you can (and will, in my experience) get good natured pedants. They’ll block a 100LOC patch for weeks because “well if we name this variable x that COULD cause someone to think of it like y so we can’t name it x” or “this pattern you used has <insert textbook downsides that generally aren’t relevant for the problem>. I would do it with this other pattern (which has its own downsides but i wont say them)”.
Which is my fundamental problem with peer review. There should be one gatekeeper to a code base that enforces some style preference. Practically which style they choose matters very little compared to having one predictable and enforced style to follow.
I kept explaining the reasoning behind my nits and built my own DB of issues, explaining why it was a problem and what to do to reference as I started to explain my points over and over to different colleagues.
I feel that just giving out what to change without context does little to teach the hard won lessons, and is everything that would annoy people who are trying to get a change in.
It is hard to fully specify all the different dimensions of consistency. Look at the code that exists and write in that voice. Maintenance will be much better.
It is even harder to chase personal consistency feelings of multiple different colleagues. Usually inconsistent with each other. Usually enforced only against less assertive committers and not enforced against those perceived to be strong.
That is why you should either make it a rule or let it be.
> Don’t review with a “how would I write it?” filter
_Thinking_ about "how would I write it?" when reviewing code catches a lot of stuff for me - I want to be able to come up with how I'd do it, see how the PR does it, and then check if the approach matches. If not, there's something to be learned and explained here. Although, all of that is in my head! If I come to the conclusion that the presented approach is wrong or inferior in an objective way (say, doesn't handle corner cases neatly or at all), I'll comment. Otherwise, let people do their thing!
So, I agree with the point of not using this to generate a lot of comments, but I do believe in it as a useful technique to engage the brain, instead of just loosely scrolling over the diff.
A navigable code base is one where people can either 1) guess where a particular bit of functionality should be implemented, 2) find a paper trail that leads them to the spot where it is.
If you can't find it, you can't fix bugs. If you don't find it, then you end up splitting related functionality across several places in the code. Which will introduce bugs, and make them a pain in the ass to diagnose.
> For many engineers - including me - it feels good to leave a blocking review, for the same reasons that it feels good to gatekeep in general.
Clearly this is not true for everyone, but I don't think I've ever felt good about gatekeeping, and a number of people I respect generally don't either. Blocking is a weapon of last resort. That someone has had to use it means either that the system isn't working, or they're abusing the tool.
Someone shouldn't arrive at the final phase of their ticket and be told to start it over. That's a retrospective item, and if it's happened multiple times then this is an agenda item at the retro meeting.
In terms of formatting etc I rely on tooling which break the CI. That way they don't even surface. Equally if something slips through the CI needs to be updated.
A piece of code should match the general style of the code it is being integrated into. When you create an addition to a wood-framed house the addition won't be steel construction, unless you have an explicit reason (weight, speed, foundation etc). And if I'm the main maintainer of a codebase, it is something I can call out on. If we use Tokio and you use raw threads I'll ask about this choice.
In an ideal world you (the PR author) includes this in its description, because it is a reflection of your understanding of the codebase.
Reviewing code with 'will this work' is also debatable. Someone might propose a change that fixes an issue they are having. But maybe it's not something we want to solve in our codebase, or maybe it breaks other things the maintainers are thinking off.
> In terms of formatting etc I rely on tooling which break the CI. That way they don't even surface. Equally if something slips through the CI needs to be updated
I also do something similar with lint rules. For Rust, this means turning on every single reasonable clippy lint (there are many), and making all warnings into errors. Some would say that some rules are overzealous or annoying, but it's a price I'm willing to pay to keep the codebase uniform.
I contributed to an OSS project recently that ran the linter before the tests and it absolutely trashed my code-build-test cycle. If I felt less strongly about the value of the project I would have bailed.
If I’m trying to get the code to work don’t cockblock me on Make it Work with your Make it Right enforcements.
That is a bit overzealous. It makes sense for CI where you don't want to waste running tests on an already failing build, but for local builds you probably want to allow warnings and be able to run tests, lints, and formatting checks separately.
I’ve heard this argument before but I don’t get it. It’s trivial to setup your code editor to format on save. Why do people not do that but rather complain about files linter checks on CI?
I don't think they were talking about formatting issues, since these tend to not change the behaviour of the code (outside of Python) so that you can't use the tests before addressing the linter issues.
Biome can fail the lint process if there’s an extra new line. Or you shortened a line that it thinks shouldn’t be wrapped anymore, or you added an argument to a line that it thinks should. I hates it.
I see where this sort of take comes from. Reviews are not a place for subjective comments that just slow down or block progress, signaling, excessive nitpicking, etc. However, the reviewer should be thinking beyond the immediate "hey, this is progress", and also consider how this change affects the codebase and how it aligns with feature and technical direction. A few happy approvals and a great codebase becomes hard to work with, if contributors are inexperienced.
Code reviews are also an opportunity for learning, and provide real-life scenarios to inform decisions that are otherwise not grounded and mostly opinion. Of course, not all comments and discussions should block approval, but there is a balance. No one is going to die if this feature is delayed by a few days if you get a better result _and_ are investing in team growth.
I'd also add: check out the branch locally. Your colleagues do some awful things to their local environments, and the central CI isn't perfect. Your IDE might have more checks enabled than both, too
I've worked in teams where I'm the only one who actually checks out the branch to poke around
I strongly disagree on this one. The CI pipeline has to be the single source of truth. Your approach is a step back to the times of "but it works on my machine".
Yes, ideally. Then again, I've caught plenty of bugs checking it out and running it locally.
The CI pipeline may not wholly be in your team's control. I've worked at places where a 'devops' team had too much control, liked to tinker all the time and often broke stuff. Nothing we could do - devops were gods and didn't take input from anyone.
Should I not do check it out locally and instead spend months and political capital arguing for more in the CI (which the PM would argue slows down velocity - and will win)?
Pragmatism and workarounds are often employed in non-bigtech enterprise companies...
edit: I also meant 'poke around' as in do a bit of informal QA. Again, your test and QA team might not be perfect...
There are times where it is reasonable, especially for bug fixes where there is a test to explicitly check that the bug is fixed. In that case, I might check out the code, revert the fix, and ensure that the test fails. There have been a few times I did that and the test still succeeded, so I can confirm that the test is not testing the right thing (in a very mature project, complexity or test settings can obscure what your test is actually doing, especially if it is an E2E test).
I have done a similar thing. I teach juniors to follow a strict test-driven development style when handling bug fixes. Write the missing, and failing, test first, then make the tests pass. (I also recommend using this approach for everything). But you can't tell if they've actually done this. It makes me think that this should be automated. Seems fairly straightforward to do in test frameworks that keep the tests in separate files. Bit harder if they're in the same files, like Rust.
There can still be value in configuring your local IDE to emit more warnings than the centralized linter does, and looking at those warnings in your IDE.
This can help you identify additional pieces of feedback that you might have missed by just reading the code in a web interface.
100%. I build web apps, and you can't automatically test every single possible interaction of a user on a page. Weird shit can very very easily sneak through without thorough manual testing.
If the code needs to work in 100 different environments it is likely that there are more bugs found, than if it is only tested in one standardised environment. The problem CI catches is, if that one environment isn't even standardised and reproducible.
In agreement with most of the points, this one was surprising to me:
> Many engineers seem to think it’s rude to leave a blocking review even if they see big problems, so they instead just leave comments describing the problems. Don’t do this. [...] Just leaving comments should mean “I’m happy for you to merge if someone else approves, even if you ignore my comments.”
Do people actually ignore a comment explaining a problem with the code as written just because it wasn't a blocking review ?
It's like ignoring someone telling you you're stepping into a hole because they're not grabbing you by the neck. Reviews shouldn't be that adversarial nor hand holding.
I'm also realizing, everywhere I work a comment is basically a blocking, except if it's explicitely flagged or discussed as optional. Trying to find some other person to review and ignore the comment is just a big NO.
Personally I hate it when people just leave comments on my PR without explicitly blocking or approving it. To me it comes across as indecisive. Either you are fine with the changes -> approve. If you think this code shouldn't be merged in it's current state -> block. Just leaving a comment feels like you want to complain, but don't really take any responsibility for what happens next.
There are exceptions of course and it all depends on the comments and the circumstances, but I generally prefer explicit yes or no.
IMHO fundamentally the MR/PR author is the one requesting reviews (be it mandatory to merge or not), and they are the ones to decide if the comments are valid/require actions or not.
If the reviewer didn't accept your PR you already know they're expecting something more, and what it is will be in the comments.
I don't know if GitHub comments can be individually marked as optional or not, but even then the reviewer might not know the actual impact of what they're pointing at.
For instance if you're deleting a bit of feature that has been already disabled at the step before, a reviewer pointing it out as blocking might as well be lacking context. I find it more natural for a comment to be mostly factual and let you check if it needs a fix or not.
It's more work on your part, but it's also your PR (all of that within reason, and not being jerks)
> Personally I hate it when people just leave comments on my PR without explicitly blocking or approving it. To me it comes across as indecisive. Either you are fine with the changes -> approve.
I point out things that aren't functional blockers all the time. If I see there's a gap from the code itself, but maybe weren't considered (is there a ticket to deal with this? I'm not going to guess what it's named or where it is), I'll point it out. If it's not blocking, it's an approval from me, but someone else may disagree when they read it as well.
Not sorry that you think it's indecisive. This is technical feedback, which is separate from product feedback. You should still read your review comments and other people's review comments. If I was in charge of things to the point only my approval matters AND I was sure the author would respond, it would be an approval. Generally, this is not the case.
Most places I’ve worked it’s fine to just comment. By approving or blocking, you signal to the team that you’re taking on the role of “the reviewer” for the code. Approving means that you have enough context to be confident in the change and stake your reputation on it. Blocking means you commit to doing subsequent rounds of review and eventually approving when your concerns are addressed (or you’re asserting a firm preference that this change should not happen).
Maybe you’re not the best person to approve the code, or you just don’t have time to commit to the reviewer role at this second, but you’ve still spotted something that you think will help the person writing the code; so, leave a comment.
Several of my coworkers complained fairly loudly that an early 'blocked' on a review makes nobody else look at it until you've cleared that problem.
If someone blocks your PR, no matter what time you believe you've finished addressing the PR comments, your PR will not go anywhere while that person is out of the office, in a meeting, or working on their own story. They've just linearized the development process.
And given the previously mentioned phenomenon, they have to be available, read your changes, unblock your PR, and then your other coworkers have to check their PR inbox, do their reviews, and then you have to make more rounds of changes.
So if a block is involved early in a review, your code basically goes through two full rounds of PR. It can lead to whiplash if the reviewers need to argue amongst themselves.
And if this is just a PR to refactor code so you can get on to the meat of your ticket, then you're blocked, not just your PR. And god help you if someone "doesn't get the point" of a PR that doesn't exhibit clear forward motion on your story. So they make it difficult to make the change easy, and then do a second PR to make the easy change.
Yep, an early block can definitely leave people stuck for a while.
Although at this point, I think something that's missing from all of these discussions (spawned from whatever ancestor comment in this thread) is what the actual policies/culture/expectations are in our respective projects.
We're all going on about things like there's one absolute truth that should be followed and that is clearly not the case.
It's not exactly the case you might be pointing at, but there will definitely be times where I don't accept but would want someone else to do so. IMHO it should happen explicitely.
For instance sometimes the translation isn't consistent with other screens, but that's not an issue I'm willing to follow to the bitter end. If that's the only thing I have concerns about, leaving a comment to check with the copy writing team and let that team approve or not is a decent course of action.
Same with security issues, queries that will cost decent money at each execution, design inconsistencies, etc.
In these cases, not approving is also less ambiguous than approving but requesting extra action that don't require additional review from us (assuming we're very explicit in the comment about being ok with the rest of the code and not needing a re-review)
Approving with comments like "please fix X before you merge" is a footgun I've decided to avoid.
I totally agree with you on being explicit about why approval isn't given.
I'll say that there are lots of things that make any/some of us suck at PR reviews that I don't think are made worse or better by this "always approve or request changes" vs "comment without approval or requesting changes is okay" difference.
What about: "It would save as effort, if you would do it like this, but having it in the current change is still better then nothing in case you are not willing to do the work."
> Do people actually ignore a comment explaining a problem with the code as written just because it wasn't a blocking review ?
I have worked with individuals who would assign NULL to a pointer, then immediately dereference it. I would bring up that this obviously would not do anything useful. The response I would get was that they already had the code working in their dev. environment, so there could not be any bugs.
> Do people actually ignore a comment explaining a problem with the code as written just because it wasn't a blocking review ?
Approval is approval. If you complete your review and don't block, you have concretely indicated to the author that in fact nothing is wrong with the diff and it is ok to be merged as is no matter what else you said. Many authors won't even look at comments if their change is approved.
Most people I know don't use it this way. Most people sign off on a review, with the expectation that the remaining comments will be addressed before pushing.
If they just push anyways, then post-push there are gonna be some awkward conversations. If it becomes common, other people would know to just not approve CRs for that person until they are pristine. It will basically just slow development speed down in the long run.
> Most people sign off on a review, with the expectation that the remaining comments will be addressed
People should be made aware that that's a bad and dangerous practice, because by doing that they are directly requesting unreviewed changes, because it means that now nobody needs to review the follow-up to their review. People need to learn to not sign off on code that they believe needs to be changed. Saying "I believe this code is wrong and needs to be fixed but am confident that the next version you produce will be completely correct without further review" is lazy reviewing and leads to inevitable disaster.
That's a bit unprofessional. The comment may be about a non-blocking issue. There is something wrong with the diff, and you should look into it.
Ignoring the comments is a tactic of a careless coworker. The diff may get merged and they can move on, sure. But come review time, they may find something else in their work environment is being rejected and removed.
On the contrary, parents comment is to me the professional variant if there is no other agreement. Every modern Code Management Platform has a distinct feature to mark a review as blocking or non-blocking, so it should be understood and communicated by the team how to use this feature.
And it should be understood that if a coworker has something to say about your code, you should listen. Anything else is obstructive and arrogant. Which are both unprofessional behaviors.
I would say that there's no such thing as a "non-blocking issue". If it's not something you want to block over, then you're saying that it's not really an issue.
Non-blocking could cover everything from “wow this is kind of jank, but it’s good enough for now and let’s figure out how we fix it later” through to very minor thing like “you have a typo”.
If we blocked on everything being perfect, we’d never get anything finished. Plus, PR’s are free.
I still fail to see how different it is from overriding a blocking review for instance.
The PR's author will always have ways around dealing with someone's feedback, and I think they should, otherwise there would be too many deadlocks (especially during incident respons etc.)
I general I prefer teams where devs give a shit and seek feedback as a positive. In my current job we allow deployment of self approved MRs in that spirit.
> Do people actually ignore a comment explaining a problem with the code as written just because it wasn't a blocking review ?
Yes, yes they do.
And some devs will go the other way - not matter how minor a comment you leave, even just those that are tagged nitpick/FYI, they must address them.
I'm in agreement with most of the points, but I still find that most of the PRs I review, I block and request changes. Maybe that says more about me and the devs I've worked with...
> Do people actually ignore a comment explaining a problem with the code as written just because it wasn't a blocking review ?
Some people leave flood of comments that cosplay as explanation, but they are not one. Or I simply disgree.
I am just getting to the point of ignoring them, because the alternative is to write an essay over each of those comments just for it to be ignored and the person passive aggressively ignoring the patch forever.
I'm a proponent of them. They help separate out atomic commits ("I'm writing the feature here... I'm doing a refactor there"). They also serve for being able to quickly identify where I need to look in the story I told myself for "what did I do there?"
While the goals that it is designed to get people to follow is something that I've preferred in my own commits in the past, this gives a structure to them that helps identify when there's too much in a commit for it to be reasonably reviewed in isolation.
- you review and if to the best of your knowledge you think something can be done better you comment about it and leave a suggestion on how to do it better
- then you approve the PR. Because your job is not to gatekeep the code
A PR can be broken, or not. If it's not broken, you approve it. You offer all the advice on how to improve it, and promise to re-approve promptly if these improvements are implemented. But you suggest it, not demand it.
If the PR is broken, you clearly denote where is it broken. I usually start comments to lines requiring changes with a red circle (U+1f354), because GitHub code review UI lacks an explicit way to mark it. You explain what needs changing, crucially, why can't it remain as is, and ideally suggest ways to fix it. Then you demand changes to the PR.
Because yes, your job is to gatekeep the codebase, protecting it from code that will definitely cause trouble. Hopefully such cases are few, but they do occur, even to best engineers at best engineering orgs.
The approver of a PR shares some responsibility in the case where the code causes production issues.
So look at the code and decide if you're willing to defend it if someone says, "Who approved this for production?" If you did your due diligence, thought the tests and the code were reasonable but some obscure interaction caused problems, you didn't have a way to know that.
If the code is just full of bad code smells and that's what blew up, then your defense is flimsy.
Production issues will happen. But they should always be the confluence of two or more errors resulting in a bad situation. Single cause failures are inexcusable.
> - then you approve the PR. Because your job is not to gatekeep the code
I can see this working when the person who wrote the code is responsible for making sure the product works for the client and that their code does not interfere with everyone else's work.
If the person reviewing though is responsible for the above it makes sense to gatekeep the code. I have been in this position before and off loading as much as possible to automated processes helps, but has never been enough or at least there is never enough time to make those automated process that would be enough.
> Review with a “will this work” filter, not with a “is this exactly how I would have done it” filter
I suppose there are edge cases where you could say "technically this will work, but when the system is close to OOM it will fail silently", but I would consider that to be a negative response for "will this work" rather than a case where you rubber stamp it.
I can't recall the last time I wrote code exactly the way I would write it. Even our compromises have compromises.
You adjust for the sort of code the rest of the team wants to see, to an extent. And you adjust for the sort of time it's reasonable to spend on a story. Even in open source I'm adjusting for that.
Occasionally, if I'm the SME on a particular section of code, it will eventually, by increments, end up being nearly exactly the sort of code I would write. And it's usually the sort of thing I do right before I leave a project. If I'm handing it over to someone else instead, I'm still going to be making it a bit more like what the new maintainer will want. If you give someone a project they suspect a trick if you don't sweeten the pot.
There is a middle ground here though. I try, anymore, to make sure I’m very clear on which review comments are blockers for me and which - most - are style, suggestions, questions. Unless the code has some egregious problem I immediately approve it and write a comment saying “this is good by me if you fix blockers A, B”; that way they are not then blocked by me doing a second review unless they want it.
Maybe this is because I’m working somewhere where we don’t use stacked reviews though? So it’s a major pain for someone to have a PR open for a long time going through lots of cycles of review, because it’s tricky to build more work on top of the in-review work
i guess there's also a difference in the places you work at.
the places i work at expect trunk to always be clean, and ready for production (continuous delivery). if you work someplace with a slower release cycle, then getting a not perfect change in may be more acceptable.
there's also responsibility, which is traced during incidents back to author and reviewers. i won't approve until i'm confident in the code, and that will mean the author needs to answer questions etc.
To some people, "I take full responsibility," means, "I will publicly admit to being wrong," instead of, "I will do everything in my power to clean up this mess that is my creation."
> If tons of PRs are being blocked, it’s usually a sign that there’s too much gatekeeping going on.
I would expand this and say if too many of the blocking statuses are coming from one reviewer, too much gatekeeping may be going on, even if the overall rate is low. Either they're going too far, or nobody else is going far enough. If you're shouting into the void, discretion is the better part of valor. You can't save people who don't want to be saved. Find people who will appreciate your efforts instead of resenting them.
I disagree with the point „don’t leave too many comments”. I think it’s easier to read a code that is a) following repo standards, b) following community standards. If a PR requires a 100 comments, so be it. Granted, I also almost always give a benefit of the doubt to the author unless a change is really required and I only ever seen a PR with close to a 100 comments once or twice. I should also say that you shouldn’t leave n comments with the same content so usually there’s no need for more than a couple comments in total.
The thing that always bothers me is when the comments involve things that have nothing to do with the PR.
Like, they are reasonable ideas, but open up a new issue. If every reviewer wants to tackle large topics in the PR Review that have nothing to do with what specifically is happening, then it explodes and gets even harder for others to review now that we are changing things that have nothing to do with the change.
It can be tricky to make a change in a file that is considered to be rotten. Especially for high priority tickets. Yes, we want to clear all of this code up. But you have to be very careful not to punish people for trying.
I had to tell one guy to knock it off because I was the only person brave enough to touch certain files and he was quickly making me regret trying by marking blocking comments on things I already planned to address in the subsequent or following PR. But I have to keep the old and new stuff working at the same time, boyo, so tap the brakes.
I think a lot of this stems from the code review tools themselves. Especially the "Don't just review the diff" mistake. Especially with GitHub defaulting to just showing a list of diffs and changed files. I'd find it much more useful if a review tool started with a class hierarchy or similar high level view to get a sense of:
1. What/how many classes changed?
2. What/how many methods have been added/removed/modified
3. What method signatures have changed
4. What changes are covered by new tests
"Don't leave too many comments" i think can really be rethought of, don't review style and syntax. Leave that to the robots. If you're relying on other engineers to flag style problems and linting, you're just wasting everybody's time. Set up linting and style checkers and be done with it.
A martial arts instructor does not tell you everything you're doing wrong on the first day. You would quit. And justifiably so. What an asshole.
You know you've progressed in their eyes every time they start bugging you about something new. You didn't suddenly become worse at something. Rather you got good enough on some higher priority thing that they knocked it off the list and replaced it with the next item in the backlog.
You should treat code reviews similarly. It's a journey, and we are in the middle. If you keep making the same comments on reviews, eventually they'll get addressed beforehand and you can point out something else.
I feel like a useful tool someone should build now that LLMs are so capable, is some sort of automated walk through of a pull request, where it steps a reviewer through initially the overview and they why of the changes, then step by step through each change, with commentary along the way generated by the LLM (and hopefully reviewed by the pull requester for accuracy). Then the reviewer can leave comments along the way.
I’ve always found the cognitive overhead of going through a pull request challenging, seems like the paradigm can shift now with all of the new tooling available to all of us.
It seems like maybe you should just talk to your colleague who wrote the code. It would be far more efficient than having them create and review a whole review experience on top of the work that they did, and then have you try to understand a walkthrough document (that might have hallucinated explanations that the requester didn’t quite catch) plus the underlying code, plus still have to talk to them to ask clarifying questions anyway.
This sounds like you're making an argument against pull request descriptions in general. Like you just want to see code changes and have a meeting about it.
I didn’t say anything about pull request descriptions.
Most of the time, I imagine that people on a team know what each other is generally doing, and that everyone is broadly familiar with the codebase. So, if you really need a ground up walkthrough of a pull request, then that is a time to talk to your colleague, because they’ve either done something brilliant or something weird. That colleague has asked you to engage with their code by tagging you in the review. If you can’t ask a question like “Alice, what am I looking at here?”, then there’s a problem.
It also seems weird to me that you seem to need to set a meeting with someone to talk about a pull review. Do you not just regularly talk to people on your team about what you guys are doing? Like, you just randomly see pull request review notifications pop up and that’s your only interaction with your team? It’s not like we talk about every detail of everything, but if something came up in a pull request that I wasn’t sure about, it would just come up in conversation.
> I didn’t say anything about pull request descriptions.
But that's what we're talking about. This is what you criticized:
automated walk through of a pull request, where it steps a reviewer through initially the overview and they why of the changes, then step by step through each change, with commentary along the way generated by the LLM (and hopefully reviewed by the pull requester for accuracy)
This is effectively what a PR description should do. It should explain what changes are being proposed and why. And having comments alongside the code changes only enhances that description IMO.
> a ground up walkthrough
Nobody said anything about a ground up walkthrough. It's walking through the changes, not the entire codebase.
> It also seems weird to me that you seem to need to set a meeting with someone to talk about a pull review
Again, I never said anything about scheduling a meeting. An ad-hoc discussion is a meeting too.
And sure, if I have a question after reading the PR then I'll ask the author. But it's certainly not the first thing I want to do.
Sure, and PR descriptions and comments are a way to do that (async).
But IME it helps to start with a good description of what the changes are rather than just having a pile of code changes dumped on you where you then have to reverse engineer the intent.
It seems draining to me to have to have to start from nothing and interrogate someone for every PR that comes in.
> You can also talk to people without scheduling a meeting
If you are leaving a comment it’s because someone didn’t learn how things should be from the tools in your codebase.
Naming style problem? Linting isn’t catching it.
Architecture violation? Your import linting (checkers to prevent route handlers importing the DB and skipping the service layer, etc.) isn’t catching it.
Duplicate method? What tooling can you create in the LLM era - remembering you can ask agents to build scripts now?
> When you receive a review with a hundred comments, it’s very hard to engage with that review on anything other than a trivial level.
Wow, that seems crazy. I can only hope I never have to work with somebody who thinks it is productive to leave that many comments on a change -- I genuinely cannot imagine any change that could ever require that.
I've had some PRs that would have required a ton of comments. There are usually two ways I handle this:
If the PR went in a completely different direction and missed the goal by a lot, I take ownership of it (with a brief explanation) and re-implement it. I then use the new PR for a pairing session, where I walk through both PRs with the original author for learning purposes.
If it’s mostly smaller issues, I schedule a half-hour pairing session with the author and review everything together, after preparing a list of issues.
Doing it any other way puts too much burden on the author to guess what the reviewer wants, and it slows down velocity significantly.
In those situations, the more productive option to course-correct is to talk to the change author in a meeting/chat instead of just volleying off a tsunami of comments about various minutiae in the change IMO.
I'll say this is directly proportionate to the size of the changes. If a code review is a conversation between n parties, it seems reasonable to me that more code leads to more comments.
I'm not convinced code review should be about "consistency" or "system memory" at all. Half of the dysfunction in large codebases comes from reviewers over-optimizing for long-term purity instead of short-term progress.
Code review is the job, in a lot of ways - whether it’s code you wrote, code a coworker wrote, or code generated by an LLM - you are asked “is it okay if this code joins the codebase?)
I work with a person where I know going into a review that I have to check every single file path. They have this weird ability to consistently put things in entirely the wrong folder/namespace. For example, something related to Categories might end up under Orders somehow... it's like new files just go in whatever folder they happen to have open at the time.
I frankly don't even know they do it so consistently.
Something to keep in mind in your reviews as well I guess, lol.
That said, it's also important the reviewer feels equally responsible for getting the work done and is willing to make pragmatic tradeoffs taking into account timeline pressure, how contained something is, how likely it is to actually cause trouble, how hard it is to change later. Cross-team and cross-org reviews are more likely to involve pure gatekeeping, which is death.
The reasons to block merging code should be explicitly and clearly stated in your Coding Standards documentation.
Things that would be personal taste in a greenfield project become important for maintenance in a mature product.
I wonder sometimes if these arguments in software are less of an issue of lack of care for craft, and more just a fundamental misunderstanding that the sorts of arguments and compromises one makes with a roommate over what state the apartment is allowed to be in are simply not analogs for a professional setting.
I get the very distinct feeling sometimes that my mis en place rules are being received as if I'm whining about whether coasters get put away when not in use, and how long old magazines get left on the couch.
Nearly all of the fastidiousness I can manage in a day I save for work, so I'm very familiar with the other side of this conversation outside of the office.
You don't get to have your own personal taste of the moment when you are reviewing; you have to broadly refer to the documented collection of tastes.
It also depends where you draw the line. For example, is dependency injection a matter of taste or a fundamental design decision for a project? What about "defensive" coding practices, like `0 == x` over `x == 0` in C?
The GMail outage of 2010, where Google had to restore 10% of GMail accounts from tape backup, was precisely because of this. A C++ migration script had used the "x = 0" construct, which set the variable to false and made the script go haywire. So not a theoretical concern. The other major takeaway from this was to always treat your migration scripts like production code and use the same warnings, tests, and defensive programming that you would use everywhere.
This is one of the biggest traps I’ve seen in code review. Generally, everyone is coming from a good place of “I’m reviewing this code to maintain codebase quality. This technically could cause problems. Thus I’m obligated to mention it”. Since the line of “could cause problems (important enough to mention)” is subjective, you can (and will, in my experience) get good natured pedants. They’ll block a 100LOC patch for weeks because “well if we name this variable x that COULD cause someone to think of it like y so we can’t name it x” or “this pattern you used has <insert textbook downsides that generally aren’t relevant for the problem>. I would do it with this other pattern (which has its own downsides but i wont say them)”.
Some traits can only be seen clearly in the rear view window.
There is not. There are a variety of personal tastes and they all conflict with each other.
A small example adding an item to a list, it might be preference whether you use “push” or “append”
Don’t care which, but it is maddening to have a unit of code that mixes them.
There are all these choices that are numerous that should be consistent, but too numerous to add to a standards document ahead of time.
What is taken by one as taste may be for another a hard won lesson in neglect or negligence.
I feel that just giving out what to change without context does little to teach the hard won lessons, and is everything that would annoy people who are trying to get a change in.
That is why you should either make it a rule or let it be.
I want to nitpick on
> Don’t review with a “how would I write it?” filter
_Thinking_ about "how would I write it?" when reviewing code catches a lot of stuff for me - I want to be able to come up with how I'd do it, see how the PR does it, and then check if the approach matches. If not, there's something to be learned and explained here. Although, all of that is in my head! If I come to the conclusion that the presented approach is wrong or inferior in an objective way (say, doesn't handle corner cases neatly or at all), I'll comment. Otherwise, let people do their thing!
So, I agree with the point of not using this to generate a lot of comments, but I do believe in it as a useful technique to engage the brain, instead of just loosely scrolling over the diff.
If you can't find it, you can't fix bugs. If you don't find it, then you end up splitting related functionality across several places in the code. Which will introduce bugs, and make them a pain in the ass to diagnose.
Clearly this is not true for everyone, but I don't think I've ever felt good about gatekeeping, and a number of people I respect generally don't either. Blocking is a weapon of last resort. That someone has had to use it means either that the system isn't working, or they're abusing the tool.
Someone shouldn't arrive at the final phase of their ticket and be told to start it over. That's a retrospective item, and if it's happened multiple times then this is an agenda item at the retro meeting.
This is exactly how I feel. The only times I would block a PR is if I know it will either:
a- actually break something existing
b- not deliver what the PR says it should deliver (eg new feature but the feature doesn't actually work)
It's basically saying "I'm certain that if we merge this, something bad will happen and we'll either cause an incident or need a hotfix".
Any of these things are a negative commentary on your process, or the members of your team.
When one finds oneself standing in a hole, it is important to stop digging.
Stuff happens. I wouldn't exactly go running to the incident/retro unless there was repeated pattern.
In terms of formatting etc I rely on tooling which break the CI. That way they don't even surface. Equally if something slips through the CI needs to be updated.
A piece of code should match the general style of the code it is being integrated into. When you create an addition to a wood-framed house the addition won't be steel construction, unless you have an explicit reason (weight, speed, foundation etc). And if I'm the main maintainer of a codebase, it is something I can call out on. If we use Tokio and you use raw threads I'll ask about this choice.
In an ideal world you (the PR author) includes this in its description, because it is a reflection of your understanding of the codebase.
Reviewing code with 'will this work' is also debatable. Someone might propose a change that fixes an issue they are having. But maybe it's not something we want to solve in our codebase, or maybe it breaks other things the maintainers are thinking off.
I also do something similar with lint rules. For Rust, this means turning on every single reasonable clippy lint (there are many), and making all warnings into errors. Some would say that some rules are overzealous or annoying, but it's a price I'm willing to pay to keep the codebase uniform.
If I’m trying to get the code to work don’t cockblock me on Make it Work with your Make it Right enforcements.
And then there’s the duplication and differences between the runscripts and the GitHub actions…
One of them uses tabs. Fucking Richard.
Code reviews are also an opportunity for learning, and provide real-life scenarios to inform decisions that are otherwise not grounded and mostly opinion. Of course, not all comments and discussions should block approval, but there is a balance. No one is going to die if this feature is delayed by a few days if you get a better result _and_ are investing in team growth.
I've worked in teams where I'm the only one who actually checks out the branch to poke around
The CI pipeline may not wholly be in your team's control. I've worked at places where a 'devops' team had too much control, liked to tinker all the time and often broke stuff. Nothing we could do - devops were gods and didn't take input from anyone.
Should I not do check it out locally and instead spend months and political capital arguing for more in the CI (which the PM would argue slows down velocity - and will win)?
Pragmatism and workarounds are often employed in non-bigtech enterprise companies...
edit: I also meant 'poke around' as in do a bit of informal QA. Again, your test and QA team might not be perfect...
A great idea, though; when we finally get automated testing in CI, I'll certainly suggest it :)
This can help you identify additional pieces of feedback that you might have missed by just reading the code in a web interface.
The worst part is the false sense of security that you get that will bite your team the second you miss reviewing a change or go on vacation
> Many engineers seem to think it’s rude to leave a blocking review even if they see big problems, so they instead just leave comments describing the problems. Don’t do this. [...] Just leaving comments should mean “I’m happy for you to merge if someone else approves, even if you ignore my comments.”
Do people actually ignore a comment explaining a problem with the code as written just because it wasn't a blocking review ?
It's like ignoring someone telling you you're stepping into a hole because they're not grabbing you by the neck. Reviews shouldn't be that adversarial nor hand holding.
I'm also realizing, everywhere I work a comment is basically a blocking, except if it's explicitely flagged or discussed as optional. Trying to find some other person to review and ignore the comment is just a big NO.
If the reviewer didn't accept your PR you already know they're expecting something more, and what it is will be in the comments.
I don't know if GitHub comments can be individually marked as optional or not, but even then the reviewer might not know the actual impact of what they're pointing at.
For instance if you're deleting a bit of feature that has been already disabled at the step before, a reviewer pointing it out as blocking might as well be lacking context. I find it more natural for a comment to be mostly factual and let you check if it needs a fix or not.
It's more work on your part, but it's also your PR (all of that within reason, and not being jerks)
I point out things that aren't functional blockers all the time. If I see there's a gap from the code itself, but maybe weren't considered (is there a ticket to deal with this? I'm not going to guess what it's named or where it is), I'll point it out. If it's not blocking, it's an approval from me, but someone else may disagree when they read it as well.
Not sorry that you think it's indecisive. This is technical feedback, which is separate from product feedback. You should still read your review comments and other people's review comments. If I was in charge of things to the point only my approval matters AND I was sure the author would respond, it would be an approval. Generally, this is not the case.
Maybe you’re not the best person to approve the code, or you just don’t have time to commit to the reviewer role at this second, but you’ve still spotted something that you think will help the person writing the code; so, leave a comment.
Blocking means "I don't approve and no one else should either."
If someone blocks your PR, no matter what time you believe you've finished addressing the PR comments, your PR will not go anywhere while that person is out of the office, in a meeting, or working on their own story. They've just linearized the development process.
And given the previously mentioned phenomenon, they have to be available, read your changes, unblock your PR, and then your other coworkers have to check their PR inbox, do their reviews, and then you have to make more rounds of changes.
So if a block is involved early in a review, your code basically goes through two full rounds of PR. It can lead to whiplash if the reviewers need to argue amongst themselves.
And if this is just a PR to refactor code so you can get on to the meat of your ticket, then you're blocked, not just your PR. And god help you if someone "doesn't get the point" of a PR that doesn't exhibit clear forward motion on your story. So they make it difficult to make the change easy, and then do a second PR to make the easy change.
Although at this point, I think something that's missing from all of these discussions (spawned from whatever ancestor comment in this thread) is what the actual policies/culture/expectations are in our respective projects.
We're all going on about things like there's one absolute truth that should be followed and that is clearly not the case.
That just sucks... because with that mindset typically nobody approves and leaves the submitter begging for approvals.
For instance sometimes the translation isn't consistent with other screens, but that's not an issue I'm willing to follow to the bitter end. If that's the only thing I have concerns about, leaving a comment to check with the copy writing team and let that team approve or not is a decent course of action.
Same with security issues, queries that will cost decent money at each execution, design inconsistencies, etc.
In these cases, not approving is also less ambiguous than approving but requesting extra action that don't require additional review from us (assuming we're very explicit in the comment about being ok with the rest of the code and not needing a re-review)
I totally agree with you on being explicit about why approval isn't given.
I'll say that there are lots of things that make any/some of us suck at PR reviews that I don't think are made worse or better by this "always approve or request changes" vs "comment without approval or requesting changes is okay" difference.
I have worked with individuals who would assign NULL to a pointer, then immediately dereference it. I would bring up that this obviously would not do anything useful. The response I would get was that they already had the code working in their dev. environment, so there could not be any bugs.
Approval is approval. If you complete your review and don't block, you have concretely indicated to the author that in fact nothing is wrong with the diff and it is ok to be merged as is no matter what else you said. Many authors won't even look at comments if their change is approved.
If they just push anyways, then post-push there are gonna be some awkward conversations. If it becomes common, other people would know to just not approve CRs for that person until they are pristine. It will basically just slow development speed down in the long run.
People should be made aware that that's a bad and dangerous practice, because by doing that they are directly requesting unreviewed changes, because it means that now nobody needs to review the follow-up to their review. People need to learn to not sign off on code that they believe needs to be changed. Saying "I believe this code is wrong and needs to be fixed but am confident that the next version you produce will be completely correct without further review" is lazy reviewing and leads to inevitable disaster.
Ignoring the comments is a tactic of a careless coworker. The diff may get merged and they can move on, sure. But come review time, they may find something else in their work environment is being rejected and removed.
Non-blocking could cover everything from “wow this is kind of jank, but it’s good enough for now and let’s figure out how we fix it later” through to very minor thing like “you have a typo”.
If we blocked on everything being perfect, we’d never get anything finished. Plus, PR’s are free.
non blocking may be resolved if you have a strong argument for why you did it that way (most people don't, so it ends up being a code change as well).
That's where I see little need for a "blocked" state, that's already what not approved should mean, short of explicitely saying otherwise.
The PR's author will always have ways around dealing with someone's feedback, and I think they should, otherwise there would be too many deadlocks (especially during incident respons etc.)
I general I prefer teams where devs give a shit and seek feedback as a positive. In my current job we allow deployment of self approved MRs in that spirit.
Yes, yes they do.
And some devs will go the other way - not matter how minor a comment you leave, even just those that are tagged nitpick/FYI, they must address them.
I'm in agreement with most of the points, but I still find that most of the PRs I review, I block and request changes. Maybe that says more about me and the devs I've worked with...
Some people leave flood of comments that cosplay as explanation, but they are not one. Or I simply disgree.
I am just getting to the point of ignoring them, because the alternative is to write an essay over each of those comments just for it to be ignored and the person passive aggressively ignoring the patch forever.
https://conventionalcomments.org/
While the goals that it is designed to get people to follow is something that I've preferred in my own commits in the past, this gives a structure to them that helps identify when there's too much in a commit for it to be reasonably reviewed in isolation.
- you review and if to the best of your knowledge you think something can be done better you comment about it and leave a suggestion on how to do it better
- then you approve the PR. Because your job is not to gatekeep the code
If the PR is broken, you clearly denote where is it broken. I usually start comments to lines requiring changes with a red circle (U+1f354), because GitHub code review UI lacks an explicit way to mark it. You explain what needs changing, crucially, why can't it remain as is, and ideally suggest ways to fix it. Then you demand changes to the PR.
Because yes, your job is to gatekeep the codebase, protecting it from code that will definitely cause trouble. Hopefully such cases are few, but they do occur, even to best engineers at best engineering orgs.
So look at the code and decide if you're willing to defend it if someone says, "Who approved this for production?" If you did your due diligence, thought the tests and the code were reasonable but some obscure interaction caused problems, you didn't have a way to know that.
If the code is just full of bad code smells and that's what blew up, then your defense is flimsy.
Production issues will happen. But they should always be the confluence of two or more errors resulting in a bad situation. Single cause failures are inexcusable.
I can see this working when the person who wrote the code is responsible for making sure the product works for the client and that their code does not interfere with everyone else's work.
If the person reviewing though is responsible for the above it makes sense to gatekeep the code. I have been in this position before and off loading as much as possible to automated processes helps, but has never been enough or at least there is never enough time to make those automated process that would be enough.
> Review with a “will this work” filter, not with a “is this exactly how I would have done it” filter
I suppose there are edge cases where you could say "technically this will work, but when the system is close to OOM it will fail silently", but I would consider that to be a negative response for "will this work" rather than a case where you rubber stamp it.
You adjust for the sort of code the rest of the team wants to see, to an extent. And you adjust for the sort of time it's reasonable to spend on a story. Even in open source I'm adjusting for that.
Occasionally, if I'm the SME on a particular section of code, it will eventually, by increments, end up being nearly exactly the sort of code I would write. And it's usually the sort of thing I do right before I leave a project. If I'm handing it over to someone else instead, I'm still going to be making it a bit more like what the new maintainer will want. If you give someone a project they suspect a trick if you don't sweeten the pot.
Maybe this is because I’m working somewhere where we don’t use stacked reviews though? So it’s a major pain for someone to have a PR open for a long time going through lots of cycles of review, because it’s tricky to build more work on top of the in-review work
the places i work at expect trunk to always be clean, and ready for production (continuous delivery). if you work someplace with a slower release cycle, then getting a not perfect change in may be more acceptable.
there's also responsibility, which is traced during incidents back to author and reviewers. i won't approve until i'm confident in the code, and that will mean the author needs to answer questions etc.
To some people, "I take full responsibility," means, "I will publicly admit to being wrong," instead of, "I will do everything in my power to clean up this mess that is my creation."
I would expand this and say if too many of the blocking statuses are coming from one reviewer, too much gatekeeping may be going on, even if the overall rate is low. Either they're going too far, or nobody else is going far enough. If you're shouting into the void, discretion is the better part of valor. You can't save people who don't want to be saved. Find people who will appreciate your efforts instead of resenting them.
Like, they are reasonable ideas, but open up a new issue. If every reviewer wants to tackle large topics in the PR Review that have nothing to do with what specifically is happening, then it explodes and gets even harder for others to review now that we are changing things that have nothing to do with the change.
I had to tell one guy to knock it off because I was the only person brave enough to touch certain files and he was quickly making me regret trying by marking blocking comments on things I already planned to address in the subsequent or following PR. But I have to keep the old and new stuff working at the same time, boyo, so tap the brakes.
"Don't leave too many comments" i think can really be rethought of, don't review style and syntax. Leave that to the robots. If you're relying on other engineers to flag style problems and linting, you're just wasting everybody's time. Set up linting and style checkers and be done with it.
You know you've progressed in their eyes every time they start bugging you about something new. You didn't suddenly become worse at something. Rather you got good enough on some higher priority thing that they knocked it off the list and replaced it with the next item in the backlog.
You should treat code reviews similarly. It's a journey, and we are in the middle. If you keep making the same comments on reviews, eventually they'll get addressed beforehand and you can point out something else.
I’ve always found the cognitive overhead of going through a pull request challenging, seems like the paradigm can shift now with all of the new tooling available to all of us.
Most of the time, I imagine that people on a team know what each other is generally doing, and that everyone is broadly familiar with the codebase. So, if you really need a ground up walkthrough of a pull request, then that is a time to talk to your colleague, because they’ve either done something brilliant or something weird. That colleague has asked you to engage with their code by tagging you in the review. If you can’t ask a question like “Alice, what am I looking at here?”, then there’s a problem.
It also seems weird to me that you seem to need to set a meeting with someone to talk about a pull review. Do you not just regularly talk to people on your team about what you guys are doing? Like, you just randomly see pull request review notifications pop up and that’s your only interaction with your team? It’s not like we talk about every detail of everything, but if something came up in a pull request that I wasn’t sure about, it would just come up in conversation.
But that's what we're talking about. This is what you criticized:
automated walk through of a pull request, where it steps a reviewer through initially the overview and they why of the changes, then step by step through each change, with commentary along the way generated by the LLM (and hopefully reviewed by the pull requester for accuracy)
This is effectively what a PR description should do. It should explain what changes are being proposed and why. And having comments alongside the code changes only enhances that description IMO.
> a ground up walkthrough
Nobody said anything about a ground up walkthrough. It's walking through the changes, not the entire codebase.
> It also seems weird to me that you seem to need to set a meeting with someone to talk about a pull review
Again, I never said anything about scheduling a meeting. An ad-hoc discussion is a meeting too.
And sure, if I have a question after reading the PR then I'll ask the author. But it's certainly not the first thing I want to do.
Sure, and PR descriptions and comments are a way to do that (async).
But IME it helps to start with a good description of what the changes are rather than just having a pile of code changes dumped on you where you then have to reverse engineer the intent.
It seems draining to me to have to have to start from nothing and interrogate someone for every PR that comes in.
> You can also talk to people without scheduling a meeting
I didn't say anything about scheduling.
That's what I intended that to mean. You can talk to your colleagues even if you have PRs.
If you are leaving a comment it’s because someone didn’t learn how things should be from the tools in your codebase.
Naming style problem? Linting isn’t catching it.
Architecture violation? Your import linting (checkers to prevent route handlers importing the DB and skipping the service layer, etc.) isn’t catching it.
Duplicate method? What tooling can you create in the LLM era - remembering you can ask agents to build scripts now?
Wow, that seems crazy. I can only hope I never have to work with somebody who thinks it is productive to leave that many comments on a change -- I genuinely cannot imagine any change that could ever require that.
Great article, fully agree with all the points.
If the PR went in a completely different direction and missed the goal by a lot, I take ownership of it (with a brief explanation) and re-implement it. I then use the new PR for a pairing session, where I walk through both PRs with the original author for learning purposes.
If it’s mostly smaller issues, I schedule a half-hour pairing session with the author and review everything together, after preparing a list of issues.
Doing it any other way puts too much burden on the author to guess what the reviewer wants, and it slows down velocity significantly.
I'll say this is directly proportionate to the size of the changes. If a code review is a conversation between n parties, it seems reasonable to me that more code leads to more comments.
It’s no small responsibility.
This hits home.
I frankly don't even know they do it so consistently.
Something to keep in mind in your reviews as well I guess, lol.