6 ms·

PR review is probably at least a little performative. But I trust my colleagues to do good reviews when I ask them to, and to ignore my PRs when I don't. That'

PR review is probably at least a little performative. But I trust my colleagues to do good reviews when I ask them to, and to ignore my PRs when I don't. That's kind of the way we all want it. I regularly ask for a review of specific changes by tagging them in a comment on the lines in question, with a description of the implications and a direct question that they can answer. This, "throw the code at the wall for interpretation" style PR just seems like it's doomed to become lower priority for busy folks. You can add a fancy narrative to the comments if you like, but the root problem is that presenting a change block as-is and asking for a boolean blanket approval of the whole thing is an expensive request. There's no substitute for shared understanding of the context and intent, and that should come out-of-band of the code in question.

pards 1y ago
> and to ignore my PRs when I don't PRs should be optional, IMHO. Not all changes require peer review, and if we trust our colleagues then we should allow them to merge their branch without wasting time with performative PRs.
- fidotron 1y ago
  There is a difference between a code review and approval to merge/release. Part of the difference is the idea you can catch all problems with piecemeal code review is nonsense, so you should have at least some sweeping QA somewhere.
- flir 1y ago
  I always appreciate an extra pair of eyeballs, even on a one-liner. Everyone's an idiot sometimes.
  - eCa 1y ago
    I’m firmly in this boat too. If it’s a small change I can likely get it reviewed within minutes, if it isn’t small it should have a review regardless.
- comprev 1y ago
  Trust, but verify. We're only human after all :-) At $DAY_JOB we need approvals from peers due to industry regulation.
  - goosejuice 1y ago
    In my experience, US healthcare, that box can be checked at later stages, namely deployment to production. It's a choice to add it earlier.
    - eCa 1y ago
      If it is for checking a box, sure. If it is part of a process that aspires to deliver projects with quality and with somewhat predictable release dates, that seems way too late, imho.
      - goosejuice 1y ago
        Sadly it often is box checking, code review or not. I'm only stating that there is no requirement in US healthcare that I'm aware of that requires approvals before merging code. Maybe that's not true in other industries. But most regulatory frameworks that I'm aware of are flexible, ambiguous, on implementation details by design. If you find that outcomes are the same by making approvals optional at that stage, then do so with accompanied justification.
      - t-writescode 1y ago
        And a great way to end up leaking customer data from a SQL injection or other error that could have easily been caught during a more piece-wise analysis and vetting of the related code nearer to time of writing.
- gagabity 1y ago
  Yes! I once read a great article I can no longer find that talked about 3 types of PRs. Simple ones that you self approve. Ones that you tag someone because you want to spread the knowledge of what has been done. And ones that need actual review. Everything being reviewed is simply unnecessary and exhausting.
- t-writescode 1y ago
  SOX compliance audit looks suspiciously at this comment. No single person being able to make changes to a system is a tenant of that.
balamatom 1y ago
>that should come out-of-band of the code in question. Ideally, yes. After a decade and something' under ZIRP, seems a lot of workers never had incentive to remain conscious of their intents in context long enough to conduct productive discourse about them. Half of the people I've worked with would feel slighted not by the bitterness the previous sentence, but by its length. There's an impedance mismatch between what actually works, and what we're expected to expect to work. That's another immediate observation which people to painfully syntaxerror much more frequently than it causes them to actually clarify context and intent. In that case the codebase remains the canonical representation of the context and intent of all contributors, even when they're not at their best, and honestly what's so bad about that? Maybe communicating them in-band instead of out-of-band might be a degraded experience. But when out-of-band can't be established, what else is there to do? I'd be happy to see tools that facilitate this sort of communication through code. GitHub for example is in perfect position to do something like that and they don't. Git + PRs + Projects implement the exact opposite information flow, the one whose failure modes people these days do whole conference talks about.
xmcqdpt2 1y ago
This is a huge pet peeve of mine. At work I'm an expert on part of the code base that sees a fair number of contributions.I get many private IMs from colleagues asking me to "Approve please" or something like that with a dump of 100s of lines, of which maybe 10 lines are relevant to me (touch files or behaviour that I'm an expert on, hence why they need my approval.) Minimally, I would like context for the change, why it required a change to this part of the codebase, and the thought process behind the change. Sometimes but not often enough I send the review back and ask them for this info. IMO many software developers are just not fast enough at writing or language so providing an explanation for their changes is a lot of work for them. Or they are checked out and they just followed the AI or IDE until things worked, so they don't have explanations to provide. People not taking the time is what makes reviews performative.
- dijksterhuis 1y ago
  > Minimally, I would like context for the change, … … the why is important > IMO many software developers … don't have explanations to provide. People not taking the time is what makes reviews performative. … a lot of developers only consider the how. i’ve had a lot of experiences of “once my PR is submitted that’s my work/ticket finished” kind of attitude. i spent a year mentoring some people in a gaming community to become dev team members. one of the first things i said about PRs was — a new PR is just your first draft, there is still more work to do. it helped these folks were brand spanking new to development and weren’t sullied by some lazy teacher somewhere.
  - stronglikedan 1y ago
    > … the why is important > … a lot of developers only consider the how. The why is someone else's job, so the developers should just ask them for a blurb to put in the PR for context, along with a note to the reviewer to ask that person for even more context if necessary.
    - pas 1y ago
      I think there's a why with regard to the code. Why this "how" and not some other "how". (Why did you pick this algorithm, this pattern, this solution to the bigger business why.)
Entelligence25 1y ago
[flagged]
Entelligence25 1y ago
[flagged]
vinnymac 1y ago
For any PR above a few line change, if a developer has not done a self review, I don’t review it all. Instead I request that it is self reviewed with context added, prior to requesting re-review. I also tend to ask the question, “are any of these insights worth adding as comments directly to the code?” 9/10 the context they wrote down should be well thought out comments in the code itself. People are incredibly lazy sometimes, even unintentionally so. We need better lint tools to help the monkey brain perform better. I wish git platforms offered more UX friendly ways to force this kind of compliance. You can kind of fake it with CI/CD but that’s not good enough imo.
- tcoff91 1y ago
  By self review, you mean that the developer adds comments in the code review tool? that is a great idea, I want to try this.
  - DerArzt 1y ago
    I've been doing this as part of my workflow for a few years now. My coworkers have expressed appreciation around that effort. A nice side effect is that going through a self review and adding comments to the PR has helped me catch innumerable things that my coworkers never had to call me on.
    - bornfreddy 1y ago
      Do you see any added value in adding the comments? I just fix the original commits and force push (each dev owns their branches at $JOB), but I'm wondering if I'm missing something?
      - DerArzt 1y ago
        The value I get is that it helps me catch errors before I ask others to check my work, and I use my PR comments as a teaching tool (I'm one of the seniors on the team).
  - goosejuice 1y ago
atoav 1y ago
Exactly, if you want people to think about your code/changes you should be able to give them the needed context. If you don't know them, please realize your code isn't automatically a gift everybody waited for, you may see it that way, but from the other side this isn't clear until someone put in the work to figure out what you did. In short: added code produces work. So the least you should do is try reducing that work by making it easy to figure out what your code is and isn't. Sum up what changes you made (functionally), why you made them, which choices you made (if any) and why and what the state of the PR code is in your own opinion. Maybe a reasoning why it is needed, what future maintenance may look like (ideally low). In essence, ask yourself what you'd like to know if someone appeared at the door and gave you a thumb drive with a patch for your project and add that knowledge. Also consider to add a draft PR for bigger features early on. This way you can avoid programming things that nobody wanted, or someone else was already working on. You also give maintainers a way to steer the direction and/or decline before you put in the work.

pards 1y ago

> and to ignore my PRs when I don't PRs should be optional, IMHO. Not all changes require peer review, and if we trust our colleagues then we should allow them to merge their branch without wasting time with performative PRs.

fidotron 1y ago
There is a difference between a code review and approval to merge/release. Part of the difference is the idea you can catch all problems with piecemeal code review is nonsense, so you should have at least some sweeping QA somewhere.
flir 1y ago
I always appreciate an extra pair of eyeballs, even on a one-liner. Everyone's an idiot sometimes.
- eCa 1y ago
  I’m firmly in this boat too. If it’s a small change I can likely get it reviewed within minutes, if it isn’t small it should have a review regardless.
comprev 1y ago
Trust, but verify. We're only human after all :-) At $DAY_JOB we need approvals from peers due to industry regulation.
- goosejuice 1y ago
  In my experience, US healthcare, that box can be checked at later stages, namely deployment to production. It's a choice to add it earlier.
  - eCa 1y ago
    If it is for checking a box, sure. If it is part of a process that aspires to deliver projects with quality and with somewhat predictable release dates, that seems way too late, imho.
    - goosejuice 1y ago
      Sadly it often is box checking, code review or not. I'm only stating that there is no requirement in US healthcare that I'm aware of that requires approvals before merging code. Maybe that's not true in other industries. But most regulatory frameworks that I'm aware of are flexible, ambiguous, on implementation details by design. If you find that outcomes are the same by making approvals optional at that stage, then do so with accompanied justification.
    - t-writescode 1y ago
      And a great way to end up leaking customer data from a SQL injection or other error that could have easily been caught during a more piece-wise analysis and vetting of the related code nearer to time of writing.
gagabity 1y ago
Yes! I once read a great article I can no longer find that talked about 3 types of PRs. Simple ones that you self approve. Ones that you tag someone because you want to spread the knowledge of what has been done. And ones that need actual review. Everything being reviewed is simply unnecessary and exhausting.
t-writescode 1y ago
SOX compliance audit looks suspiciously at this comment. No single person being able to make changes to a system is a tenant of that.

balamatom 1y ago

>that should come out-of-band of the code in question. Ideally, yes. After a decade and something' under ZIRP, seems a lot of workers never had incentive to remain conscious of their intents in context long enough to conduct productive discourse about them. Half of the people I've worked with would feel slighted not by the bitterness the previous sentence, but by its length. There's an impedance mismatch between what actually works, and what we're expected to expect to work. That's another immediate observation which people to painfully syntaxerror much more frequently than it causes them to actually clarify context and intent. In that case the codebase remains the canonical representation of the context and intent of all contributors, even when they're not at their best, and honestly what's so bad about that? Maybe communicating them in-band instead of out-of-band might be a degraded experience. But when out-of-band can't be established, what else is there to do? I'd be happy to see tools that facilitate this sort of communication through code. GitHub for example is in perfect position to do something like that and they don't. Git + PRs + Projects implement the exact opposite information flow, the one whose failure modes people these days do whole conference talks about.

xmcqdpt2 1y ago

This is a huge pet peeve of mine. At work I'm an expert on part of the code base that sees a fair number of contributions.I get many private IMs from colleagues asking me to "Approve please" or something like that with a dump of 100s of lines, of which maybe 10 lines are relevant to me (touch files or behaviour that I'm an expert on, hence why they need my approval.) Minimally, I would like context for the change, why it required a change to this part of the codebase, and the thought process behind the change. Sometimes but not often enough I send the review back and ask them for this info. IMO many software developers are just not fast enough at writing or language so providing an explanation for their changes is a lot of work for them. Or they are checked out and they just followed the AI or IDE until things worked, so they don't have explanations to provide. People not taking the time is what makes reviews performative.

dijksterhuis 1y ago
> Minimally, I would like context for the change, … … the why is important > IMO many software developers … don't have explanations to provide. People not taking the time is what makes reviews performative. … a lot of developers only consider the how. i’ve had a lot of experiences of “once my PR is submitted that’s my work/ticket finished” kind of attitude. i spent a year mentoring some people in a gaming community to become dev team members. one of the first things i said about PRs was — a new PR is just your first draft, there is still more work to do. it helped these folks were brand spanking new to development and weren’t sullied by some lazy teacher somewhere.
- stronglikedan 1y ago
  > … the why is important > … a lot of developers only consider the how. The why is someone else's job, so the developers should just ask them for a blurb to put in the PR for context, along with a note to the reviewer to ask that person for even more context if necessary.
  - pas 1y ago
    I think there's a why with regard to the code. Why this "how" and not some other "how". (Why did you pick this algorithm, this pattern, this solution to the bigger business why.)

Entelligence25 1y ago

[flagged]

Entelligence25 1y ago

[flagged]

vinnymac 1y ago

For any PR above a few line change, if a developer has not done a self review, I don’t review it all. Instead I request that it is self reviewed with context added, prior to requesting re-review. I also tend to ask the question, “are any of these insights worth adding as comments directly to the code?” 9/10 the context they wrote down should be well thought out comments in the code itself. People are incredibly lazy sometimes, even unintentionally so. We need better lint tools to help the monkey brain perform better. I wish git platforms offered more UX friendly ways to force this kind of compliance. You can kind of fake it with CI/CD but that’s not good enough imo.

tcoff91 1y ago
By self review, you mean that the developer adds comments in the code review tool? that is a great idea, I want to try this.
- DerArzt 1y ago
  I've been doing this as part of my workflow for a few years now. My coworkers have expressed appreciation around that effort. A nice side effect is that going through a self review and adding comments to the PR has helped me catch innumerable things that my coworkers never had to call me on.
  - bornfreddy 1y ago
    Do you see any added value in adding the comments? I just fix the original commits and force push (each dev owns their branches at $JOB), but I'm wondering if I'm missing something?
    - DerArzt 1y ago
      The value I get is that it helps me catch errors before I ask others to check my work, and I use my PR comments as a teaching tool (I'm one of the seniors on the team).
- goosejuice 1y ago

atoav 1y ago

Exactly, if you want people to think about your code/changes you should be able to give them the needed context. If you don't know them, please realize your code isn't automatically a gift everybody waited for, you may see it that way, but from the other side this isn't clear until someone put in the work to figure out what you did. In short: added code produces work. So the least you should do is try reducing that work by making it easy to figure out what your code is and isn't. Sum up what changes you made (functionally), why you made them, which choices you made (if any) and why and what the state of the PR code is in your own opinion. Maybe a reasoning why it is needed, what future maintenance may look like (ideally low). In essence, ask yourself what you'd like to know if someone appeared at the door and gave you a thumb drive with a patch for your project and add that knowledge. Also consider to add a draft PR for bigger features early on. This way you can avoid programming things that nobody wanted, or someone else was already working on. You also give maintainers a way to steer the direction and/or decline before you put in the work.