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.

Ahhh solid yup sounds like we're on the same page then thanks for the extra explanation ! :)