7 ms·

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 p

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.

sevenseacat 11mo ago
> 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...
- hinkley 11mo ago
  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.
BugsJustFindMe 11mo ago
> 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.
- JoeAltmaier 11mo ago
  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.
  - siva7 11mo ago
    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.
    - JoeAltmaier 11mo ago
      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.
      - BugsJustFindMe 11mo ago
        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.
        1718627440 11mo ago
        > 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.
watwut 11mo ago
> 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.
- makeitdouble 11mo ago
  I feel for you, I'd do whatever in my power to get out of that kind of toxicity.
papanoah 11mo ago
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.
- rkomorn 11mo ago
  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."
  - dakiol 11mo ago
    > 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.
    - rkomorn 11mo ago
      This does not improve with someone blocking the PR.
      - papanoah 11mo ago
        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!'.
deleted 11mo ago
[deleted]
sidewndr46 11mo ago
> 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.

sevenseacat 11mo ago

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

hinkley 11mo ago
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.

BugsJustFindMe 11mo ago

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

JoeAltmaier 11mo ago
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.
- siva7 11mo ago
  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.
  - JoeAltmaier 11mo ago
    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.
    - BugsJustFindMe 11mo ago
      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.
      - 1718627440 11mo ago
        > 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.

watwut 11mo ago

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

makeitdouble 11mo ago
I feel for you, I'd do whatever in my power to get out of that kind of toxicity.

papanoah 11mo ago

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.

rkomorn 11mo ago
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."
- dakiol 11mo ago
  > 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.
  - rkomorn 11mo ago
    This does not improve with someone blocking the PR.
    - papanoah 11mo ago
      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!'.

deleted 11mo ago

[deleted]

sidewndr46 11mo ago

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