5 ms·

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

> 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 1y 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 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
grogers 1y ago
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.
- BugsJustFindMe 1y ago
  > 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.
makeitdouble 1y ago
But the MR/PR is not approved, just commented. That's where I see little need for a "blocked" state, that's already what not approved should mean, short of explicitely saying otherwise.
- BugsJustFindMe 1y ago
  In a group reviewing scenario, someone else will approve it, and then you have no recourse. If you don't approve, block.
  - makeitdouble 1y ago
    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.
    - BugsJustFindMe 1y ago
      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.
      - 1718627440 1y ago
        > Not blocking is a signal that you have no objection to merging the change as is. Only when that is you social agreement. To others this has the meaning of "objection, but not rejection".
        BugsJustFindMe 1y ago
        If it makes you feel better, just think of ignoring non-blocking comments as equivalent to reading them and disagreeing.

JoeAltmaier 1y 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 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

grogers 1y ago

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.

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

makeitdouble 1y ago

But the MR/PR is not approved, just commented. That's where I see little need for a "blocked" state, that's already what not approved should mean, short of explicitely saying otherwise.

BugsJustFindMe 1y ago
In a group reviewing scenario, someone else will approve it, and then you have no recourse. If you don't approve, block.
- makeitdouble 1y ago
  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.
  - BugsJustFindMe 1y ago
    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.
    - 1718627440 1y ago
      > Not blocking is a signal that you have no objection to merging the change as is. Only when that is you social agreement. To others this has the meaning of "objection, but not rejection".
      - BugsJustFindMe 1y ago
        If it makes you feel better, just think of ignoring non-blocking comments as equivalent to reading them and disagreeing.