4 ms·

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 c

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 1y 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 1y 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 1y 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 1y 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.
      - BugsJustFindMe 1y ago
        > 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.
        1718627440 1y ago
        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.
BugsJustFindMe 1y ago
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.
- arccy 1y ago
  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).
- FridgeSeal 1y ago
  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.
  - dns_snek 1y ago
    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.
    - 1718627440 1y ago
      Put it into a comment or into one of todo.txt, features.txt, ideas.txt, under docs/ or in README?
      - dns_snek 1y ago
        What's the point of leaving a review comment then?
        1718627440 1y ago

siva7 1y 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 1y 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 1y 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 1y 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.
    - BugsJustFindMe 1y ago
      > 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.
      - 1718627440 1y ago
        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.

BugsJustFindMe 1y ago

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.

arccy 1y ago
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).
FridgeSeal 1y ago
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.
- dns_snek 1y ago
  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.
  - 1718627440 1y ago
    Put it into a comment or into one of todo.txt, features.txt, ideas.txt, under docs/ or in README?
    - dns_snek 1y ago
      What's the point of leaving a review comment then?
      - 1718627440 1y ago