[papanoah]

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.

[makeitdouble]

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)

[Supermancho]

> 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.

[titanomachy]

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.

[rkomorn]

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."

[hinkley]

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.

[rkomorn]

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.

[dakiol]

> 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.

[makeitdouble]

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)

[rkomorn]

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.

[rkomorn]

This does not improve with someone blocking the PR.

[papanoah]

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!'.

[rkomorn]

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".

[AppleBananaPie]

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 :)

[1718627440]

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."

[watwut]

Then approve. That is an approval.

[1718627440]

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".

[watwut]

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.

[1718627440]

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.

[FridgeSeal]

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.

[watwut]

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.

[makeitdouble]

> 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.

[watwut]

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.