3 ms·

I appreciate suggestions that are not "directly related to a necessary change to the code", and I believe my coworkers appreciate mine. It is very useful to say

I appreciate suggestions that are not "directly related to a necessary change to the code", and I believe my coworkers appreciate mine. It is very useful to say "did you consider doing it xyz way / using xyz library?" while also mashing the "approve" button. If they say "yep and I decided not to because of xyz" or "yep I like that idea but I'm not gonna change it at this point" then no harm done. But pretty often in my experience they say, "ah, nice, thanks, I didn't think of that / I wasn't aware of that, I like that more; updated now", and then everybody is even happier. Another useful thing, in my opinion, is to use your "fresh eyes" to make suggestions on what could use to be clarified in code or documentation. "This wasn't immediately obvious to me, could you add a comment on why it works this way?". The author is often too close to the code to see that it isn't obvious, because it is to them, because they've already spent hours thinking about it. But this should almost always what be optional suggestions. (But, rarely, code is so unclear that it really shouldn't go in without being clarified or documented.) But in general I do think it's important to consistently be thinking to oneself "my goal is to be useful to my team and organization. are my comments achieving that? would it be more useful to approve or to not approve this right now?". The temptation to be a fastidious editor can be strong - if most of us didn't have this impulse we'd probably be doing different jobs... - but I find this "usefulness" framing to help me a lot in resisting it. And very strong disagree on not requiring reviews. It is much better to tirelessly foster a low-friction review culture.

_ea1k 3y ago
> [...] while also mashing the "approve" button +1 - It is easy to forget that it is possible to approve with comments. Not all review feedback should block a merge.
- nox100 3y ago
  It is a merge block in some code bases. You need to know the code the person said they were going to commit is the code they actually commit. Especially when there is financial incentive and state actors that want code inserted. In my project we used to be allowed to approve with nits but recently they changed it that the code needs a re-review for almost any edits. The system has some criteria for which it will allow minor edits but it's strict enough that I've rarely seen it pass an edit. I assume there must have been some incident that triggered this.
  - lokar 3y ago
    IME this is always driven by lazy compliance people pushing the most straight forward way to satisfy some audit. Plenty of places subject to strict audit rules manage to make small post review edits work.
  - dkersten 3y ago
    I read it as it’s up to the person who opened the PR whether to apply the nits, or whether to ignore them and use the approval to merge. This is how our codebase works: you’re free to ignore minor suggestions, but if you do edit the code, reviews are dismissed and need to be provided again. Basically, an unaddressed nit doesn’t block merging. But any code changes will require fresh reviews. If the change is very minor, it’s very quick for someone who has already approved to check the last commit diff and reapprove.
  - yjftsjthsd-h 3y ago
    I think it's legitimate to force a re-review for any code change, given that programming is one of those charming areas where a single character can completely change the meaning of something. That said, if you can read the commits individually then final review for something where minor changes were added should be utterly trivial.
  - _ea1k 3y ago
    Yes, it is pretty common to require re-review if it is changed again. But approval with comments shifts control back to the submitter. They then have choices: - Modify it and accept that this will trigger re-review - Merge as-is... maybe they don't even agree with the nit - Merge as-is and followup with a new MR under the same JIRA. This sometimes makes sense if it gets them into an important preprod environment, but they are really still working on the issue. - Merge as-is and follow up with a JIRA on the backlog. Maybe they realized that the "nit" is really a bigger issue and should be addressed separately. Regardless, there's value in not blocking the merge for non-functional issues.
    - lazyasciiart 3y ago
      I got a merge blocked last week with the comment 'I think this work is more related to "TFS X: prepare for Y" than it is "TFS Y".' (I eventually realized they wanted me to change the commit message). I hate this fucking team.

_ea1k 3y ago

> [...] while also mashing the "approve" button +1 - It is easy to forget that it is possible to approve with comments. Not all review feedback should block a merge.

nox100 3y ago
It is a merge block in some code bases. You need to know the code the person said they were going to commit is the code they actually commit. Especially when there is financial incentive and state actors that want code inserted. In my project we used to be allowed to approve with nits but recently they changed it that the code needs a re-review for almost any edits. The system has some criteria for which it will allow minor edits but it's strict enough that I've rarely seen it pass an edit. I assume there must have been some incident that triggered this.
- lokar 3y ago
  IME this is always driven by lazy compliance people pushing the most straight forward way to satisfy some audit. Plenty of places subject to strict audit rules manage to make small post review edits work.
- dkersten 3y ago
  I read it as it’s up to the person who opened the PR whether to apply the nits, or whether to ignore them and use the approval to merge. This is how our codebase works: you’re free to ignore minor suggestions, but if you do edit the code, reviews are dismissed and need to be provided again. Basically, an unaddressed nit doesn’t block merging. But any code changes will require fresh reviews. If the change is very minor, it’s very quick for someone who has already approved to check the last commit diff and reapprove.
- yjftsjthsd-h 3y ago
  I think it's legitimate to force a re-review for any code change, given that programming is one of those charming areas where a single character can completely change the meaning of something. That said, if you can read the commits individually then final review for something where minor changes were added should be utterly trivial.
- _ea1k 3y ago
  Yes, it is pretty common to require re-review if it is changed again. But approval with comments shifts control back to the submitter. They then have choices: - Modify it and accept that this will trigger re-review - Merge as-is... maybe they don't even agree with the nit - Merge as-is and followup with a new MR under the same JIRA. This sometimes makes sense if it gets them into an important preprod environment, but they are really still working on the issue. - Merge as-is and follow up with a JIRA on the backlog. Maybe they realized that the "nit" is really a bigger issue and should be addressed separately. Regardless, there's value in not blocking the merge for non-functional issues.
  - lazyasciiart 3y ago
    I got a merge blocked last week with the comment 'I think this work is more related to "TFS X: prepare for Y" than it is "TFS Y".' (I eventually realized they wanted me to change the commit message). I hate this fucking team.