> 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.
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.
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.
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.
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.
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.
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)”.
> 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.
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.
> 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'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.
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.
It sends a different message, in my opinion. Blocking means "I disagree, but lets figure it out and work together to get it over the finish line". "I don't approve, but someone else can" is very non-commital. Which gives me the feeling of being left alone with a bunch of critique, without appreciation for the work that I have originally done. I would wish my reviewer takes responsibility for his/her feedback.
"I don't approve, but someone else can" also means to me "Merge it, if you must. If it works out, good for you, I havent blocked it. If it doesnt work out, I get to say 'See, I told you so!'.
Having non-blocking comments leaves room for the discussion you want.
It's your job as the PR submitter to advocate for your code and shepherd it through.
Either you, indeed, work with the reviewer who made the comments to resolve them, or you have the option to seek out another if you think the feedback isn't valid enough to address.
Edit: TBH I don't get why you'd see a non-blocking comment differently, eg not meaning "let's get this done".
If a system requires a sign off for a PR to be submitted then it's a collaborative effort for the PR to succeed.
Someone just leaving comments and not signing off on reviews isn't helping unblock anyone and should put in more effort to be willing to sign off and move the work forward. If the most people in the org thought this way nothing would be committed and everyone would have 'non-blocking' comments to deal with.
Another way to look at this is in absence of another code reviewer, not signing off after commenting is equivalent to passively blocking the PR and can be a bit toxic depending on the circumstance.
I'm probably missing a scenario (maybe there's a bunch of people you know will review the code for instance) that this makes sense so happy to learn where/when specifically it makes sense :)
> not signing off after commenting is equivalent to passively blocking the PR and can be a bit toxic depending on the circumstance
Blocking a PR can also be toxic "depending on the circumstance".
I see zero toxicity in leaving comments without blocking. It's never prevented the people I've worked with from getting work done.
I've worked at three large tech companies and none of them had this "block PRs" mentality but they all got stuff done. The reviewers understand their roles: they leave feedback, if there are questions, they answer them. If the feedback's handled, they re-review.
It works exactly the way you say it should, minus the "blocked/changes requested" status on a PR. Maybe precisely because we understand that a PR is blocked until it's approved anyway, and the green check is the goal.
All the opportunities for dysfunction are the same: people can still bikeshed, they can not review, they can not come back and re-review, etc. None of that is affected by the "changes requested" vs "comment" dichotomy.
Frankly, the "we can't collaborate without blocking PRs" take seems strangely dysfunctional to me.
I think I don't understand the last sentence. This seems the opposite of what I wrote ?
As for leaving comments without blocking I do not mean it is always or even commonly toxic but that I've seen instances where it could be argued to be so or potentially unhelpful.
I think the misunderstanding might be when you or your team leave comments without blocking are you going to sign off after they are addressed or are you leaving them on a review you ultimately don't feel comfortable signing off on even if they're addressed?
How often does someone leave comments on a review they would never feel comfortable signing off on either way because they don't know the area? I think I'm in agreement with you - leaving comments without blocking and signing off after they're addressed or if someone else signs off and mine aren't addressed that's fine. I'd block the review if it was something I was that concerned with.
> I think I don't understand the last sentence. This seems the opposite of what I wrote ?
I guess I misunderstood, and I think I attributed some context from others' previous comments to you. My bad, sorry. :) Looks like we generally agree.
When we leave comments, even without blocking, we're going to sign off when they're addressed (assuming someone else doesn't sign off first). That's our job as reviewers.
If we don't feel comfortable signing off (eg: because the diffs also touch an area outside our knowledge) then we just comment to that effect. ie "this part LGTM, but someone else from <team X> needs to sign off."
The main thing is: if we have comments on a PR that we think should be addressed, but aren't "do not merge this under any circumstances", then we just don't select the "request changes" option, and it doesn't seem to cause problems for us.
That said, if I worked somewhere where there was established guidance to either accept or request changes, then I'd do that without a second thought.
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."
I don't want you to write an essay; I want to point out, that there is a third state, besides block and approve. I don't want to never have that code in case, the author disagrees with me or I am missing something.
Teams I have worked in often have an Approved subject to comments” with th “all comments must be resolved” marker set.
This somewhat violates the articles “approval or not is the only bit that matters” argument, but it at least forces participants to at least acknowledge an issue, and either provide a satisfactory reason to resolve on the spot, or open a new ticket for that work.
That was addressed by someone else: https://news.ycombinator.com/item?id=45705759 . Also that just sounds like comments with extra steps. What is the meaning of plain comments in these teams?
Doing "something", that solves the issue that is described in the comment?
That can range from actually changing the code, putting a comment in, over updating the documentation, moving the code somewhere else, to merging the PR unchanged and opening a new PR, to not doing anything.
> You want to block me, but also want it to look like my decision instead of yours.
Because it IS your decision. What the comment means for the effect on your PR depends on how you address it. "changing the code" -> "new code review"; "putting a comment in", "updating the documentation" -> don't need approval, but expect questions; "not doing anything" -> get your approval from someone else, you won't get it from me, but if someone else is fine with it, maybe I am wrong.
I DO NOT want to block you, that's why I want to be able to only leave a comment.
Examples:
"This conflicts with POSIX and SUSv4 ..." -> (but the old code also did) "document that diversion and leave a point in todo.txt"
"This is highly surprising for users" -> "add a warning or a confirmation"
"This breaks the workflow for X people (including me), but this was undocumented behaviour" -> "Consider whether the feature is worth it and we care about this"
"This conflicts with my understanding of RFC1234 in combination with 'random blog post from The Old New Thing' and documentation in 'different vendor's codebase'" -> "Yes, I know, I did it on purpose, because of footnote in RFC5678 and random comment on HN proving why Raymond Chen is wrong; will document that".
The answer to all these comments could also be "Oops, didn't thought of that, now I need to change the code / throw everything away.", but this entirely depends on your state of mind, which I don't have introspection to. If I would start researching that problem in depth, then I could also do the PR myself, that is work, that you already did. I only want to make sure, that you are aware. If you are, consider it approved, if you are not, consider it blocked.
It will feel efficient for the two people involved, but I'd actually want that exchange to be public and in writing, as we're talking about code that will go to production and will be read, debugged and maintained by dozen of other people from there.
Especially if it was complex enough to be discussed to these lengths. I don't care about code comments and don't trust people to properly maintain them, as long as MRs have the original context and discussion of the decisions. Shorting these discussions is a net loss to the org IMHO.
No one will read that discussion, ever. And it is such an epic waste of time to write down all these nitpicky discussions.
And a call where we react to each other takes 3 minutes max while the exchange through comments can take a week. Or, 2 days but both of you need to interrupt what you are doing a lot to answer quickly.
It is just absurd to treat code review as a discussion forum.
I spend half of my life on a legacy codebase, and the past PRs are my go to to understand anything mildly tricky in there.
My biggest realization was that the code comments were meaningless and borderline deceptive compared to the actual discussions and the target specs in the MRs. There's such a gap between what people want to explain or think will be useful, and the info actually needed. And I don't blame them, I'm not sure I'd do better.
My favorite is threads like "This name makes no sense" -> "Sure, give me a better name for X doing Y to Z" -> "Nevermind", which completely explains some of the insane naming, what they actually meant by it, and would never be left in any half official documentation.
"This name makes no sense" -> "Sure, give me a better name for X doing Y to Z" -> "Nevermind"
Should really result in a comment "/* X doing Y to Z */" before the declaration. I try to address these issues by treating PRs the same as phone calls, they can be gone tomorrow, so I put all the things in the code or the commit message. But of course I am not perfect, the people after me will determine whether I was successful.
> 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.
It's far more arrogant to expect/demand changes without blocking than it is for the author to take the reviewer at their word about whether the change needs additional work or not.
By leaving a non-blocking review, the coworker has said that they have no strong objection to merging the code as is.
If it makes you feel better, just think of ignoring non-blocking comments as equivalent to reading them and disagreeing.
That's why you are at work and get compensated. I don't need to motivate you to do your work, that's something you should discuss with your boss.
> it is for the author to take the reviewer at their word about whether the change needs additional work or not.
How can the reviewer know that? The reviewer can only point at issues that tangent their part of the code, or what problems they think this will cause. Whether this is intentional or accident can only be known by the author.
> By leaving a non-blocking review, the coworker has said that they have no strong objection to merging the code as is.
No they say e.g. they would be know they need to work around the issue on their side, but prefer no to, because this comes at a costs for the company in work-hours and whether that is something you want to cause is your choice.
It seems like you think code reviews are a way to put someone else in charge of your change and deflecting the blame to him. You are still responsible for your work. Code reviews prevent you from accidentally breaking things, reducing the error rate by having more eyes and knowledge looking at your code. They won't prevent you from willfully breaking things.
> think of ignoring non-blocking comments as equivalent to reading them and disagreeing.
This is fine and expected, but we already have that discussion elsewhere.
> because this comes at a costs for the company in work-hours
Their additional work on updating the code also comes at a cost for the company in work-hours. If you suspect that the cost of your work-around is greater than the cost to update their code and you don't block, you're the one being negligent.
Yes, and if I do know that for sure, I should block the PR. Getting insights on the exact problems the PR addresses is what the Author already did and it would be wasteful and micromanaging from me if I would try to do that again only to find out whether this is the case here or not.
My non-blocking comment is exactly a symptom of me being unable to decide whether I would block or accept it. I just ask for the author to improve the PR so this can be decided on.
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.
Yep, or make a ticket in whatever issue/ticket/work management system you use. Everywhere I’ve worked has always had some kind of “future stuff we’d like to do” project.
But then rather than leaving a non-blocking review, why not leave a blocking review and approve it as soon as the author responds with a link to the ticket or adds it to TODO.md?
If it's important enough to be tracked then I'd want to make sure that it's been filed properly. It's too easy to forget about such a trivial task, especially in a time crunch.
Because it belongs in to the PR and only the Author knows the answer: his thoughts and rationale for the change and behaviour. Sure I could just guess, but then it's essentially poking in legacy code. The point of it being not legacy code is, that the person is still around, so he should write down his reasons, so that they are documented.
Sure, what I'm trying to say is that you need to design a process which ensures that these non-blocking review comments get organized properly, and enforce it. If you need the author to add your comments to TODO.md then that should once again become a blocking review.
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.
There's a massive difference in communicated intent between overriding a blocking review and not blocking at all. Not blocking is a signal that you have no objection to merging the change as is.
If you care about whether your concerns are addressed by further discussion, block. If you don't actually care about whether something happens as indicated by not blocking, it's passive aggressive to expect more work.
People forget that you can just as easily block pending responses to comments and then approve after.
Sure, ok, so then they're addressed by the decision to ignore them because you don't care enough about them to make them blocking and there are other things to work on.
I think your understanding of "ignore them" is different than mine. When I say ignore them I mean ignore them, not do more labor to document a thought process in response to them which is the opposite of ignoring them.
If you refuse to do something on a problem I raise, one of us should be fired. Either you, because you refuse to work for what you get money, or me because I hand out advice on random stuff I am not supposed to do.
If you did ask me for my opinion, but ignore them, that's a refusal to work on your side. If you didn't asked me and I still started to mess with your work, then either I did found a serious issue and you should be grateful, if you ignore that, that is misconduct, or I spread nonsense around and should get a meeting with the employer on why I harass my colleagues.
Creating documentation is the professional way of ignoring problems. If I don't do that, I am acting against the profession I am employed as. Sweeping evidence of problems under the rack, will be later classified as something from at best misconduct to at worst malicious and criminal.
> If you refuse to do something on a problem I raise
If you think there's a problem and you don't block, then you're the one being negligent. If you review without blocking, you're indicating that you see no reasons why the code shouldn't be merged as is.
> If you did ask me for my opinion, but ignore them
In this scenario I saw that you don't see anything that needs to change. That is a direct expression of opinion from you by reviewing and not blocking. I am following your input efficiently by disregarding optional side quests you want to invoke.
The problem in this scenario is that you want to say "I do not think you need to do anything else", which is what it means to review and not block, while simultaneously expecting the person to do something else. Your contradictory input is what creates the problem here.
> you want to say "I do not think you need to do anything else", which is what it means to review and not block
> you're indicating that you see no reasons why the code shouldn't be merged as is
> In this scenario I saw that you don't see anything that needs to change.
This is a definition you introduced and I started the thread with saying that I disagree with this meaning. "I do not see anything that needs to change/you need to do" is indicated by me accepting the PR or just not leaving a comment at all. Me saying "you should consider fixing that/ bringing your code in concordance with Spec XY", is certainly not a case of that.
> If you think there's a problem and you don't block, then you're the one being negligent.
If I know for sure, that this is a real problem I would do that, but it's still you who choose to do a breaking change, after having known of a problem.
> optional side quests you want to invoke
If someone would want to put optional side quests in my PR, I would tell them to update the specification or create their own PR, but leave my PR alone.
You essentially say that you will only fix issues, when your colleagues force you to address them, by preventing you from merging code changes unless you fix them, and not just because you know your code is buggy. This doesn't sound like a healthy work culture to me. It should be in your interest to make your code non-buggy, not for your colleagues to need to force you to do that. Also this doesn't scale at all. Do you do the same to your colleagues? Then this seems like a weird workflow. Or do you expect this to be a one-way street?
So essentially I understand this be like this dialog:
A: Hi, I implemented that new feature ... Can you have a look.
B: Looks mostly fine... Are you sure line 18 is not a bug, because ...
A: No it isn't./ Oops, it is.
[If there is enough time A can explain this, or just do the fix and talk about it during lunch break.]
B: Ok, let's merge this.
I understand your dialog to go:
A: Hi, I implemented that new feature ... Can you have a look.
B: Looks mostly fine... Are you sure line 18 is not a bug, because ...
A: Force me to deal with it, otherwise I will just merge.
You obviously don't care about that problem, otherwise you would have told me, that the RFC I read yesterday is superseded,
because you can read my mind and know which RFC I had in mind when I wrote that.
B: ?????
A PR is a formalization of the above dialog, because most people are lazy and don't have that dialog on their own. In a PR, A doesn't need to answer whether A considers that to be a bug, because that's normally implicit in whether A has fixed that or not. With A from the second dialog this obviously doesn't work, but I think in this case no workflow will bring A to care about the code having no bugs, and this is behaviour I would describe as something from misconduct to malicious.
If you believe that line 18 may be a bug, not blocking is your negligence. It is your duty as the reviewer to block pending resolution of things that you believe may be bugs. Since we're talking about jobs, that is your job, not theirs. Otherwise you are saying "I believe you wrote code that may cause problems, but I'm not willing to do anything about it", which is being a lazy reviewer.
> 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."
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.
There’s no way a PR description should be expected to have a step by step description of what the change is doing, along with a commentary. That’s what I mean by “ground up”: explaining every line of code with its thinking is something that maybe you’d do to teach coding or something.
If a dev has to take time even to review and edit an LLM generated version of this for every pull review (and it will require time to do this so that it doesn’t waste the reviewer’s time with wild goose chases due to faulty interpretation), and you are then going to have to wade through that doc in addition to reading the code, you could save everyone a lot of time and just talk to each other when you have questions.
I'm not looking at it like a line by line explanation of the code. Think of it like commit messages, but better. Better because:
1. commit messages are usually not very informative and the context is implicit.
2. commit messages are coupled to time rather than to the final PR changes. The PR changes are what really matters to the reviewer, not a log of what the author did to get there (especially if things are changing back and forth).
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.
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.
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?)
> 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.
> Blocking is a weapon of last resort
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".
c - it represents a complete failure to adhere to a standard of coding that we all approved as a group.
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.
Agreed on c- . The rest seems a bit strident.
Stuff happens. I wouldn't exactly go running to the incident/retro unless there was repeated pattern.
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.
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.
There is a major exception to the personal tastes issue, consistency and predictably in a mature codebase.
Things that would be personal taste in a greenfield project become important for maintenance in a mature product.
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 are encoded in conventions which are someone's personal taste.
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.
If there was one personal taste, this is achievable.
There is not. There are a variety of personal tastes and they all conflict with each other.
None of us mature in the same directions at the same speed.
What is taken by one as taste may be for another a hard won lesson in neglect or negligence.
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.
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.
To clarify, I am talking about more than surface level style.
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.
Then make that explicit topic during the setup instead of taking random commits hostage to whatever catches your attention this week.
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.
You should absolutely enforce any patterns that showed up in RCA findings and the results of any R&D that has found better patterns.
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?
> Maturity is something to be observed, not declared.
Some traits can only be seen clearly in the rear view window.
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:
to: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.
> the definitions are cloudy enough […]
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)”.
The time to impose your personal taste is when you get to be the author, co-author or contributor to your organization's coding conventions documents.
Great article, and I mostly agree.
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.
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.
I'm a little stricter.
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.
Same here, I have a Cargo.toml in which I painstakingly went through every lint and decided whether it made sense to me.
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 will probably file a PR to flip them around. Since the runscript will stop on a broken test it’ll work better for local.
And then there’s the duplication and differences between the runscripts and the GitHub actions…
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.
It’s trivial to set up my code editor to save to the format of six different GitHub repos? Do tell.
One of them uses tabs. Fucking Richard.
Your code editor should respect the .editorconfig of the repo you are editing, so yeah, trivial.
Bold of you to assume aggravating projects have an editorconfig. (Checked, the worst doesn't)
Only one does as it turns out.
> 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'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).
That can be an automated CI check that runs the new tests with the non-test files reverted though.
That wouldn't necessarily compile though, eg if the fix involved changing a function signature or adding new functions.
It can be! And maybe even should be. But I've never seen it.
A great idea, though; when we finally get automated testing in CI, I'll certainly suggest it :)
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.
Yup, same here
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.
In my mind you are wasting time and enabling your team to get away with not fix your CI.
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
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.
The way I look at it, commenting without approval means "I don't approve (and here's why) but someone else can."
Blocking means "I don't approve and no one else should either."
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.
> I don't approve (and here's why) but someone else can
That just sucks... because with that mindset typically nobody approves and leaves the submitter begging for approvals.
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.
This does not improve with someone blocking the PR.
It sends a different message, in my opinion. Blocking means "I disagree, but lets figure it out and work together to get it over the finish line". "I don't approve, but someone else can" is very non-commital. Which gives me the feeling of being left alone with a bunch of critique, without appreciation for the work that I have originally done. I would wish my reviewer takes responsibility for his/her feedback. "I don't approve, but someone else can" also means to me "Merge it, if you must. If it works out, good for you, I havent blocked it. If it doesnt work out, I get to say 'See, I told you so!'.
I don't see it that way.
Having non-blocking comments leaves room for the discussion you want.
It's your job as the PR submitter to advocate for your code and shepherd it through.
Either you, indeed, work with the reviewer who made the comments to resolve them, or you have the option to seek out another if you think the feedback isn't valid enough to address.
Edit: TBH I don't get why you'd see a non-blocking comment differently, eg not meaning "let's get this done".
If a system requires a sign off for a PR to be submitted then it's a collaborative effort for the PR to succeed.
Someone just leaving comments and not signing off on reviews isn't helping unblock anyone and should put in more effort to be willing to sign off and move the work forward. If the most people in the org thought this way nothing would be committed and everyone would have 'non-blocking' comments to deal with.
Another way to look at this is in absence of another code reviewer, not signing off after commenting is equivalent to passively blocking the PR and can be a bit toxic depending on the circumstance.
I'm probably missing a scenario (maybe there's a bunch of people you know will review the code for instance) that this makes sense so happy to learn where/when specifically it makes sense :)
> not signing off after commenting is equivalent to passively blocking the PR and can be a bit toxic depending on the circumstance
Blocking a PR can also be toxic "depending on the circumstance".
I see zero toxicity in leaving comments without blocking. It's never prevented the people I've worked with from getting work done.
I've worked at three large tech companies and none of them had this "block PRs" mentality but they all got stuff done. The reviewers understand their roles: they leave feedback, if there are questions, they answer them. If the feedback's handled, they re-review.
It works exactly the way you say it should, minus the "blocked/changes requested" status on a PR. Maybe precisely because we understand that a PR is blocked until it's approved anyway, and the green check is the goal.
All the opportunities for dysfunction are the same: people can still bikeshed, they can not review, they can not come back and re-review, etc. None of that is affected by the "changes requested" vs "comment" dichotomy.
Frankly, the "we can't collaborate without blocking PRs" take seems strangely dysfunctional to me.
I think I don't understand the last sentence. This seems the opposite of what I wrote ?
As for leaving comments without blocking I do not mean it is always or even commonly toxic but that I've seen instances where it could be argued to be so or potentially unhelpful.
I think the misunderstanding might be when you or your team leave comments without blocking are you going to sign off after they are addressed or are you leaving them on a review you ultimately don't feel comfortable signing off on even if they're addressed?
How often does someone leave comments on a review they would never feel comfortable signing off on either way because they don't know the area? I think I'm in agreement with you - leaving comments without blocking and signing off after they're addressed or if someone else signs off and mine aren't addressed that's fine. I'd block the review if it was something I was that concerned with.
> I think I don't understand the last sentence. This seems the opposite of what I wrote ?
I guess I misunderstood, and I think I attributed some context from others' previous comments to you. My bad, sorry. :) Looks like we generally agree.
When we leave comments, even without blocking, we're going to sign off when they're addressed (assuming someone else doesn't sign off first). That's our job as reviewers.
If we don't feel comfortable signing off (eg: because the diffs also touch an area outside our knowledge) then we just comment to that effect. ie "this part LGTM, but someone else from <team X> needs to sign off."
The main thing is: if we have comments on a PR that we think should be addressed, but aren't "do not merge this under any circumstances", then we just don't select the "request changes" option, and it doesn't seem to cause problems for us.
That said, if I worked somewhere where there was established guidance to either accept or request changes, then I'd do that without a second thought.
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."
Then approve. That is an approval.
But I want the comments being addressed first? The article describes approve to mean: "I’m happy for you to merge, even if you ignore my comments".
Then block. If you want them to be addressed as in change in the code, block.
If you want me to write an essay in the response comment, then freaking call or something.
I don't want you to write an essay; I want to point out, that there is a third state, besides block and approve. I don't want to never have that code in case, the author disagrees with me or I am missing something.
Teams I have worked in often have an Approved subject to comments” with th “all comments must be resolved” marker set.
This somewhat violates the articles “approval or not is the only bit that matters” argument, but it at least forces participants to at least acknowledge an issue, and either provide a satisfactory reason to resolve on the spot, or open a new ticket for that work.
That was addressed by someone else: https://news.ycombinator.com/item?id=45705759 . Also that just sounds like comments with extra steps. What is the meaning of plain comments in these teams?
So, what do you mean by "addressed"? Because what you are describing is not a third state, but passive aggressive block.
You want to block me, but also want it to look like my decision instead of yours.
> So, what do you mean by "addressed"?
Doing "something", that solves the issue that is described in the comment?
That can range from actually changing the code, putting a comment in, over updating the documentation, moving the code somewhere else, to merging the PR unchanged and opening a new PR, to not doing anything.
> You want to block me, but also want it to look like my decision instead of yours.
Because it IS your decision. What the comment means for the effect on your PR depends on how you address it. "changing the code" -> "new code review"; "putting a comment in", "updating the documentation" -> don't need approval, but expect questions; "not doing anything" -> get your approval from someone else, you won't get it from me, but if someone else is fine with it, maybe I am wrong.
I DO NOT want to block you, that's why I want to be able to only leave a comment.
Examples:
"This conflicts with POSIX and SUSv4 ..." -> (but the old code also did) "document that diversion and leave a point in todo.txt"
"This is highly surprising for users" -> "add a warning or a confirmation"
"This breaks the workflow for X people (including me), but this was undocumented behaviour" -> "Consider whether the feature is worth it and we care about this"
"This conflicts with my understanding of RFC1234 in combination with 'random blog post from The Old New Thing' and documentation in 'different vendor's codebase'" -> "Yes, I know, I did it on purpose, because of footnote in RFC5678 and random comment on HN proving why Raymond Chen is wrong; will document that".
The answer to all these comments could also be "Oops, didn't thought of that, now I need to change the code / throw everything away.", but this entirely depends on your state of mind, which I don't have introspection to. If I would start researching that problem in depth, then I could also do the PR myself, that is work, that you already did. I only want to make sure, that you are aware. If you are, consider it approved, if you are not, consider it blocked.
> call or something.
I kinda hate this.
It will feel efficient for the two people involved, but I'd actually want that exchange to be public and in writing, as we're talking about code that will go to production and will be read, debugged and maintained by dozen of other people from there.
Especially if it was complex enough to be discussed to these lengths. I don't care about code comments and don't trust people to properly maintain them, as long as MRs have the original context and discussion of the decisions. Shorting these discussions is a net loss to the org IMHO.
No one will read that discussion, ever. And it is such an epic waste of time to write down all these nitpicky discussions.
And a call where we react to each other takes 3 minutes max while the exchange through comments can take a week. Or, 2 days but both of you need to interrupt what you are doing a lot to answer quickly.
It is just absurd to treat code review as a discussion forum.
I spend half of my life on a legacy codebase, and the past PRs are my go to to understand anything mildly tricky in there.
My biggest realization was that the code comments were meaningless and borderline deceptive compared to the actual discussions and the target specs in the MRs. There's such a gap between what people want to explain or think will be useful, and the info actually needed. And I don't blame them, I'm not sure I'd do better.
My favorite is threads like "This name makes no sense" -> "Sure, give me a better name for X doing Y to Z" -> "Nevermind", which completely explains some of the insane naming, what they actually meant by it, and would never be left in any half official documentation.
I generally agree with you, but this:
"This name makes no sense" -> "Sure, give me a better name for X doing Y to Z" -> "Nevermind"
Should really result in a comment "/* X doing Y to Z */" before the declaration. I try to address these issues by treating PRs the same as phone calls, they can be gone tomorrow, so I put all the things in the code or the commit message. But of course I am not perfect, the people after me will determine whether I was successful.
You can get the best of both worlds by doing a phone call and then writing your insights in the commit message.
> 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.
It's far more arrogant to expect/demand changes without blocking than it is for the author to take the reviewer at their word about whether the change needs additional work or not.
By leaving a non-blocking review, the coworker has said that they have no strong objection to merging the code as is.
If it makes you feel better, just think of ignoring non-blocking comments as equivalent to reading them and disagreeing.
> It's far more arrogant to expect/demand changes
That's why you are at work and get compensated. I don't need to motivate you to do your work, that's something you should discuss with your boss.
> it is for the author to take the reviewer at their word about whether the change needs additional work or not.
How can the reviewer know that? The reviewer can only point at issues that tangent their part of the code, or what problems they think this will cause. Whether this is intentional or accident can only be known by the author.
> By leaving a non-blocking review, the coworker has said that they have no strong objection to merging the code as is.
No they say e.g. they would be know they need to work around the issue on their side, but prefer no to, because this comes at a costs for the company in work-hours and whether that is something you want to cause is your choice.
It seems like you think code reviews are a way to put someone else in charge of your change and deflecting the blame to him. You are still responsible for your work. Code reviews prevent you from accidentally breaking things, reducing the error rate by having more eyes and knowledge looking at your code. They won't prevent you from willfully breaking things.
> think of ignoring non-blocking comments as equivalent to reading them and disagreeing.
This is fine and expected, but we already have that discussion elsewhere.
> because this comes at a costs for the company in work-hours
Their additional work on updating the code also comes at a cost for the company in work-hours. If you suspect that the cost of your work-around is greater than the cost to update their code and you don't block, you're the one being negligent.
Yes, and if I do know that for sure, I should block the PR. Getting insights on the exact problems the PR addresses is what the Author already did and it would be wasteful and micromanaging from me if I would try to do that again only to find out whether this is the case here or not.
My non-blocking comment is exactly a symptom of me being unable to decide whether I would block or accept it. I just ask for the author to improve the PR so this can be decided on.
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.
This strikes me as somewhat un-pragmatic.
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.
How do you organize that information so that you can actually follow up on it later?
Writing a non-blocking review feels a bit like writing to /dev/null. Good to get it out of my system but not much else.
Put it into a comment or into one of todo.txt, features.txt, ideas.txt, under docs/ or in README?
Yep, or make a ticket in whatever issue/ticket/work management system you use. Everywhere I’ve worked has always had some kind of “future stuff we’d like to do” project.
But then rather than leaving a non-blocking review, why not leave a blocking review and approve it as soon as the author responds with a link to the ticket or adds it to TODO.md?
If it's important enough to be tracked then I'd want to make sure that it's been filed properly. It's too easy to forget about such a trivial task, especially in a time crunch.
What's the point of leaving a review comment then?
Because it belongs in to the PR and only the Author knows the answer: his thoughts and rationale for the change and behaviour. Sure I could just guess, but then it's essentially poking in legacy code. The point of it being not legacy code is, that the person is still around, so he should write down his reasons, so that they are documented.
Sure, what I'm trying to say is that you need to design a process which ensures that these non-blocking review comments get organized properly, and enforce it. If you need the author to add your comments to TODO.md then that should once again become a blocking review.
I do not want to prescribe the author what he needs to do. I only want to make sure that it was an intentional choice to not do that.
to me blocking means something about the code must change.
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).
But the MR/PR is not approved, just commented.
That's where I see little need for a "blocked" state, that's already what not approved should mean, short of explicitely saying otherwise.
In a group reviewing scenario, someone else will approve it, and then you have no recourse. If you don't approve, block.
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.
There's a massive difference in communicated intent between overriding a blocking review and not blocking at all. Not blocking is a signal that you have no objection to merging the change as is.
> Not blocking is a signal that you have no objection to merging the change as is.
Only when that is you social agreement. To others this has the meaning of "objection, but not rejection".
If it makes you feel better, just think of ignoring non-blocking comments as equivalent to reading them and disagreeing.
And this is what I think they are for. Just document that disagreement, and you have also countered future questions about this.
"no"
If you care about whether your concerns are addressed by further discussion, block. If you don't actually care about whether something happens as indicated by not blocking, it's passive aggressive to expect more work.
People forget that you can just as easily block pending responses to comments and then approve after.
> If you care about whether your concerns are addressed by further discussion, block.
Well yes, but I don't care about them being addressed by discussion, I want them being addressed by your decision.
> it's passive aggressive to expect more work.
I am not sure what exactly "passive aggressive" is supposed to mean, but you generally get paid so that others can expect more work.
> I want them being addressed by your decision
Sure, ok, so then they're addressed by the decision to ignore them because you don't care enough about them to make them blocking and there are other things to work on.
If you documented this and provide a valid rationale for ignoring the issue, then the comment did it's job.
> If you documented this and provide
I think your understanding of "ignore them" is different than mine. When I say ignore them I mean ignore them, not do more labor to document a thought process in response to them which is the opposite of ignoring them.
If you refuse to do something on a problem I raise, one of us should be fired. Either you, because you refuse to work for what you get money, or me because I hand out advice on random stuff I am not supposed to do.
If you did ask me for my opinion, but ignore them, that's a refusal to work on your side. If you didn't asked me and I still started to mess with your work, then either I did found a serious issue and you should be grateful, if you ignore that, that is misconduct, or I spread nonsense around and should get a meeting with the employer on why I harass my colleagues.
Creating documentation is the professional way of ignoring problems. If I don't do that, I am acting against the profession I am employed as. Sweeping evidence of problems under the rack, will be later classified as something from at best misconduct to at worst malicious and criminal.
> If you refuse to do something on a problem I raise
If you think there's a problem and you don't block, then you're the one being negligent. If you review without blocking, you're indicating that you see no reasons why the code shouldn't be merged as is.
> If you did ask me for my opinion, but ignore them
In this scenario I saw that you don't see anything that needs to change. That is a direct expression of opinion from you by reviewing and not blocking. I am following your input efficiently by disregarding optional side quests you want to invoke.
The problem in this scenario is that you want to say "I do not think you need to do anything else", which is what it means to review and not block, while simultaneously expecting the person to do something else. Your contradictory input is what creates the problem here.
> you want to say "I do not think you need to do anything else", which is what it means to review and not block
> you're indicating that you see no reasons why the code shouldn't be merged as is
> In this scenario I saw that you don't see anything that needs to change.
This is a definition you introduced and I started the thread with saying that I disagree with this meaning. "I do not see anything that needs to change/you need to do" is indicated by me accepting the PR or just not leaving a comment at all. Me saying "you should consider fixing that/ bringing your code in concordance with Spec XY", is certainly not a case of that.
> If you think there's a problem and you don't block, then you're the one being negligent.
If I know for sure, that this is a real problem I would do that, but it's still you who choose to do a breaking change, after having known of a problem.
> optional side quests you want to invoke
If someone would want to put optional side quests in my PR, I would tell them to update the specification or create their own PR, but leave my PR alone.
I don't really think we have the same understanding of non-blocking comments. I wrote some examples of what I think of in: https://news.ycombinator.com/item?id=45711969 .
---
You essentially say that you will only fix issues, when your colleagues force you to address them, by preventing you from merging code changes unless you fix them, and not just because you know your code is buggy. This doesn't sound like a healthy work culture to me. It should be in your interest to make your code non-buggy, not for your colleagues to need to force you to do that. Also this doesn't scale at all. Do you do the same to your colleagues? Then this seems like a weird workflow. Or do you expect this to be a one-way street?
So essentially I understand this be like this dialog:
I understand your dialog to go: A PR is a formalization of the above dialog, because most people are lazy and don't have that dialog on their own. In a PR, A doesn't need to answer whether A considers that to be a bug, because that's normally implicit in whether A has fixed that or not. With A from the second dialog this obviously doesn't work, but I think in this case no workflow will bring A to care about the code having no bugs, and this is behaviour I would describe as something from misconduct to malicious.If you believe that line 18 may be a bug, not blocking is your negligence. It is your duty as the reviewer to block pending resolution of things that you believe may be bugs. Since we're talking about jobs, that is your job, not theirs. Otherwise you are saying "I believe you wrote code that may cause problems, but I'm not willing to do anything about it", which is being a lazy reviewer.
> 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...
I can't decide if github is better or worse for making you 'resolve' all conversation whether you choose to do anything about them or not.
> 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 feel for you, I'd do whatever in my power to get out of that kind of toxicity.
Is anyone else using conventional comments
https://conventionalcomments.org/
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.
It's not that complicated:
- 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.
FTA
> 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.
words by someone who has never needed to be responsible for running code. no wonder people like ai vibecoding.
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.
Some people don't know what responsible means.
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 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.
If I see that majority of code is generated, I reject the PR
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.
There’s no way a PR description should be expected to have a step by step description of what the change is doing, along with a commentary. That’s what I mean by “ground up”: explaining every line of code with its thinking is something that maybe you’d do to teach coding or something.
If a dev has to take time even to review and edit an LLM generated version of this for every pull review (and it will require time to do this so that it doesn’t waste the reviewer’s time with wild goose chases due to faulty interpretation), and you are then going to have to wade through that doc in addition to reading the code, you could save everyone a lot of time and just talk to each other when you have questions.
Agree to disagree.
I'm not looking at it like a line by line explanation of the code. Think of it like commit messages, but better. Better because:
1. commit messages are usually not very informative and the context is implicit.
2. commit messages are coupled to time rather than to the final PR changes. The PR changes are what really matters to the reviewer, not a log of what the author did to get there (especially if things are changing back and forth).
You can have both sync and aync communication. You can also talk to people without scheduling a meeting.
> You can have both sync and aync communication
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.
> Sure, and PR descriptions and comments are a way to do that (async).
That's what I intended that to mean. You can talk to your colleagues even if you have PRs.
There is opportunity in the meta.
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.
Great article, fully agree with all the points.
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.
usually it means your pr is too big and/or you didn't discuss it enough beforehand...
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.
When the PR system is being used as a weapon to freeze someone out, everything has gone to shit.
> Many software engineers now spend as much (or more) time reviewing the output of their own AI tools than their colleagues’ code.
This hits home.
> Don’t leave too many comments
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.
This is actually good article.
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.
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?)
It’s no small responsibility.