In agreement with most of the points, this one was surprising to me:
> Many engineers seem to think it’s rude to leave a blocking review even if they see big problems, so they instead just leave comments describing the problems. Don’t do this. [...] Just leaving comments should mean “I’m happy for you to merge if someone else approves, even if you ignore my comments.”
Do people actually ignore a comment explaining a problem with the code as written just because it wasn't a blocking review ?
It's like ignoring someone telling you you're stepping into a hole because they're not grabbing you by the neck. Reviews shouldn't be that adversarial nor hand holding.
I'm also realizing, everywhere I work a comment is basically a blocking, except if it's explicitely flagged or discussed as optional. Trying to find some other person to review and ignore the comment is just a big NO.
Personally I hate it when people just leave comments on my PR without explicitly blocking or approving it. To me it comes across as indecisive. Either you are fine with the changes -> approve. If you think this code shouldn't be merged in it's current state -> block. Just leaving a comment feels like you want to complain, but don't really take any responsibility for what happens next.
There are exceptions of course and it all depends on the comments and the circumstances, but I generally prefer explicit yes or no.
IMHO fundamentally the MR/PR author is the one requesting reviews (be it mandatory to merge or not), and they are the ones to decide if the comments are valid/require actions or not.
If the reviewer didn't accept your PR you already know they're expecting something more, and what it is will be in the comments.
I don't know if GitHub comments can be individually marked as optional or not, but even then the reviewer might not know the actual impact of what they're pointing at.
For instance if you're deleting a bit of feature that has been already disabled at the step before, a reviewer pointing it out as blocking might as well be lacking context. I find it more natural for a comment to be mostly factual and let you check if it needs a fix or not.
It's more work on your part, but it's also your PR (all of that within reason, and not being jerks)
> Personally I hate it when people just leave comments on my PR without explicitly blocking or approving it. To me it comes across as indecisive. Either you are fine with the changes -> approve.
I point out things that aren't functional blockers all the time. If I see there's a gap from the code itself, but maybe weren't considered (is there a ticket to deal with this? I'm not going to guess what it's named or where it is), I'll point it out. If it's not blocking, it's an approval from me, but someone else may disagree when they read it as well.
Not sorry that you think it's indecisive. This is technical feedback, which is separate from product feedback. You should still read your review comments and other people's review comments. If I was in charge of things to the point only my approval matters AND I was sure the author would respond, it would be an approval. Generally, this is not the case.
Most places I’ve worked it’s fine to just comment. By approving or blocking, you signal to the team that you’re taking on the role of “the reviewer” for the code. Approving means that you have enough context to be confident in the change and stake your reputation on it. Blocking means you commit to doing subsequent rounds of review and eventually approving when your concerns are addressed (or you’re asserting a firm preference that this change should not happen).
Maybe you’re not the best person to approve the code, or you just don’t have time to commit to the reviewer role at this second, but you’ve still spotted something that you think will help the person writing the code; so, leave a comment.
Several of my coworkers complained fairly loudly that an early 'blocked' on a review makes nobody else look at it until you've cleared that problem.
If someone blocks your PR, no matter what time you believe you've finished addressing the PR comments, your PR will not go anywhere while that person is out of the office, in a meeting, or working on their own story. They've just linearized the development process.
And given the previously mentioned phenomenon, they have to be available, read your changes, unblock your PR, and then your other coworkers have to check their PR inbox, do their reviews, and then you have to make more rounds of changes.
So if a block is involved early in a review, your code basically goes through two full rounds of PR. It can lead to whiplash if the reviewers need to argue amongst themselves.
And if this is just a PR to refactor code so you can get on to the meat of your ticket, then you're blocked, not just your PR. And god help you if someone "doesn't get the point" of a PR that doesn't exhibit clear forward motion on your story. So they make it difficult to make the change easy, and then do a second PR to make the easy change.
Yep, an early block can definitely leave people stuck for a while.
Although at this point, I think something that's missing from all of these discussions (spawned from whatever ancestor comment in this thread) is what the actual policies/culture/expectations are in our respective projects.
We're all going on about things like there's one absolute truth that should be followed and that is clearly not the case.
It's not exactly the case you might be pointing at, but there will definitely be times where I don't accept but would want someone else to do so. IMHO it should happen explicitely.
For instance sometimes the translation isn't consistent with other screens, but that's not an issue I'm willing to follow to the bitter end. If that's the only thing I have concerns about, leaving a comment to check with the copy writing team and let that team approve or not is a decent course of action.
Same with security issues, queries that will cost decent money at each execution, design inconsistencies, etc.
In these cases, not approving is also less ambiguous than approving but requesting extra action that don't require additional review from us (assuming we're very explicit in the comment about being ok with the rest of the code and not needing a re-review)
Approving with comments like "please fix X before you merge" is a footgun I've decided to avoid.
I totally agree with you on being explicit about why approval isn't given.
I'll say that there are lots of things that make any/some of us suck at PR reviews that I don't think are made worse or better by this "always approve or request changes" vs "comment without approval or requesting changes is okay" difference.
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.
> 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.