4 ms·

What I find to be maybe the single most important part of code review is knowledge transfer. Our entire small team thumbs up a PR before it's merged unless the

by donatj 3mo ago

What I find to be maybe the single most important part of code review is knowledge transfer. Our entire small team thumbs up a PR before it's merged unless there's a big rush on it, and this gives everyone on the team a rough idea of the state of the codebase at any given time. There's no being blindsided like "this whole system I depend on is gone" like I had happen at far more siloed places I've worked. Beyond that, it gives a forum to ask questions about how things work to further build understanding. On a high functioning team, every developer should have at least a modest understanding of the entire system, including parts they never touch. Another important feature is just the institutional knowledge check. For instance recently I made a small change to a table and a coworker pointed out that there was a microservice I wasn't considering that wrote to that table that would break (yes, sharing tables is bad design, unrelated). I had no idea this microservice existed let alone had access to this table. The institutional knowledge check here though prevented a larger issue and potential data cleanup situation.

rowanseymour 3mo ago
We even find ourselves creating PRs in situations where the code is going to be merged immediately anyway, and tagging other devs, just so they have a convenient way to see what got merged and why. So people don't lose track of what is in the codebase.
- jonahx 3mo ago
  It's a good practice. Worth mentioning also: the same can be done with ordinary git log, assuming everyone is using git well. A proper git log of yesterday's work can be like your work newspaper with coffee.
  - laybak 3mo ago
    this seems like a chain of good practices. though I find it hard to stay disciplined about keeping commits well scoped and well described
    - jonahx 3mo ago
      Becoming very comfortable with "rebase --interactive" and other cmds for editing your (local!) history before merging helps a lot. Once you are, it only adds 5m or so of extra work to most PRs. And while acquiring this knowledge used to be difficult, LLMs make it very easy these days.
      - duskdozer 3mo ago
        I would also recommend an editor designed for rebases. I use the nodejs rebase-editor TUI (though it looks like the old non-vibecoded releases have been removed from github, so unclear on the current availability of this one) which makes it easier to organize
    - morganherlocker 3mo ago
      You can setup most PR systems to squash on merge using the first commit's message, then enforce that the top commit message is prefixed with a ticket ID. This practice has many benefits: readable git log, easier git bisect for tracking down regressions, it becomes easy to find all commits associated with a block of work, more useful git blame.
      - jonahx 3mo ago
        I strongly recommend against auto-squashing. It creates large commits without the semantic intent of the author. In a large PR, you want clean, small, semantic commits, which makes it much easier to review and understand.
        duskdozer 3mo ago
        I agree, though it feels like a bit of a lost cause at times to get people to make those commits
- fusslo 3mo ago
  this is a great way to train people to ignore PR emails/notifications
  - rowanseymour 3mo ago
    Maybe but those people are also having claude summarize their inbox and get nice little daily reports on things they might want to look into.
6LLvveMx2koXfwn 3mo ago
> For instance recently I made a small change to a table and a coworker pointed out that there was a microservice I wasn't considering that wrote to that table that would break If code reviews are important, where does testing sit? Presumably if the coworker had not been part of the code review something would have stopped the breaking change making its way to prod?
- chii 3mo ago
  > something would have stopped the breaking change making its way to prod? or a prod outage causes the knowledge to be experienced.
- marcosdumay 3mo ago
  I'd guess testing is done only for the software being deployed, not for that other microservice. At least, that's what people do by default.
- skydhash 3mo ago
  > If code reviews are important, where does testing sit? Testing is for general boundaries. If you have a formalized specs, investing in an harness and writing a lot of tests is worth it. But more likely in a corporate system, fully documenting everything is a huge hurdle by itself. So your best bet is to gather everyone that is related to a change and let them evaluate its impact.
- nunez 3mo ago
  Before the PR is cut, before the PR is sent to review (so that time isn't wasted on low-valuw bugs and problems --- engineers are expensive!), pre-merge and pre-release.
abtinf 3mo ago
[flagged]
abirch 3mo ago
Complexity is caused by Dependencies and Obscurities ~ John Ousterhout Looks like you had both.
thomascgalvin 3mo ago
> Our entire small team thumbs up a PR before it's merged unless there's a big rush on it, and this gives everyone on the team a rough idea of the state of the codebase at any given time. There's no being blindsided like "this whole system I depend on is gone" like I had happen at far more siloed places I've worked. How large is your team? Because I don't think that would scale beyond maybe five engineers I'm a huge proponent of automated testing, because that catches things like "this whole system I depend on is gone" even if the guy who depends on it isn't in the room I'm also a huge proponent of shared ownership of ... everything, really. It's natural for people to kind of own different pieces of a codebase, especially if it's a component they created, but that leads to silos and low bus counts. There shouldn't be one guy who owns one system that depends on one other component
koolba 3mo ago
> Our entire small team thumbs up a PR before it's merged unless there's a big rush on it, and this gives everyone on the team a rough idea of the state of the codebase at any given time. For non trivial or chore updates a second pair of eyes is always a good idea. But it’s not possible to scale out “everybody reads everything” to a large N. The problem is that nobody could keep up with that ad the reader when there are some huge number of things to read. That’s why we delegate, create docs, and have overview sessions.
Aurornis 3mo ago
> Our entire small team thumbs up a PR before it's merged unless there's a big rush on it This has been tried by a couple of my past managers. This feels great with a small team on a slow moving codebase. If you try to force it on a larger team or expect the codebase to move quickly then it turns into a performative game of skimming the code (if that) to click the thumbs up button so you can get back to your work. The end game was a situation where nobody was really reviewing code because everyone had their own work to do and they didn’t want to be the one person blocking important PRs, so everyone was clicking thumbs up.
- cucumber3732842 3mo ago
  This with a side helping of "when everyone is responsible nobody is" Then again, if you're dealing with an un-maintainable minefield of a code base then "fire the whole team, I dare you" might be what you're after.
Scubabear68 3mo ago
Based on your description, I don't think the process is working. You describe a shared database, micro services you aren't aware of using the same database, and a desire for everyone to have a rough idea of the state of the codebase. And things breaking unexpectedly being caught by code review. You have a bigger problem here of a system you can't reason about very well. The code review will help here, but I think you have bigger discoverability problems based on what I'm reading. As others have said, what is needed is automated testing that would catch this sort of problem automatically without needing a human in the loop saying "Oh, wait...". We still want/need humans in the loop, but they should not be the only safety net.
chrisra 3mo ago
Decisions by small groups should be the default. Others only need to be involved if the risk/consequences of failure are high. I started ignoring all PRs from our large team because we had a similar policy. My teammates can handle, they don't need me to check on each PR.
stronglikedan 3mo ago
> "this whole system I depend on is gone" This would be caught upstream in my org. If something was removed in a PR which was not documented and discussed with the entire team ahead of time, that would not be good for the submitter.

by donatj 3mo ago

rowanseymour 3mo ago

We even find ourselves creating PRs in situations where the code is going to be merged immediately anyway, and tagging other devs, just so they have a convenient way to see what got merged and why. So people don't lose track of what is in the codebase.

jonahx 3mo ago
It's a good practice. Worth mentioning also: the same can be done with ordinary git log, assuming everyone is using git well. A proper git log of yesterday's work can be like your work newspaper with coffee.
- laybak 3mo ago
  this seems like a chain of good practices. though I find it hard to stay disciplined about keeping commits well scoped and well described
  - jonahx 3mo ago
    Becoming very comfortable with "rebase --interactive" and other cmds for editing your (local!) history before merging helps a lot. Once you are, it only adds 5m or so of extra work to most PRs. And while acquiring this knowledge used to be difficult, LLMs make it very easy these days.
    - duskdozer 3mo ago
      I would also recommend an editor designed for rebases. I use the nodejs rebase-editor TUI (though it looks like the old non-vibecoded releases have been removed from github, so unclear on the current availability of this one) which makes it easier to organize
  - morganherlocker 3mo ago
    You can setup most PR systems to squash on merge using the first commit's message, then enforce that the top commit message is prefixed with a ticket ID. This practice has many benefits: readable git log, easier git bisect for tracking down regressions, it becomes easy to find all commits associated with a block of work, more useful git blame.
    - jonahx 3mo ago
      I strongly recommend against auto-squashing. It creates large commits without the semantic intent of the author. In a large PR, you want clean, small, semantic commits, which makes it much easier to review and understand.
      - duskdozer 3mo ago
        I agree, though it feels like a bit of a lost cause at times to get people to make those commits
fusslo 3mo ago
this is a great way to train people to ignore PR emails/notifications
- rowanseymour 3mo ago
  Maybe but those people are also having claude summarize their inbox and get nice little daily reports on things they might want to look into.

6LLvveMx2koXfwn 3mo ago

> For instance recently I made a small change to a table and a coworker pointed out that there was a microservice I wasn't considering that wrote to that table that would break If code reviews are important, where does testing sit? Presumably if the coworker had not been part of the code review something would have stopped the breaking change making its way to prod?

chii 3mo ago
> something would have stopped the breaking change making its way to prod? or a prod outage causes the knowledge to be experienced.
marcosdumay 3mo ago
I'd guess testing is done only for the software being deployed, not for that other microservice. At least, that's what people do by default.
skydhash 3mo ago
> If code reviews are important, where does testing sit? Testing is for general boundaries. If you have a formalized specs, investing in an harness and writing a lot of tests is worth it. But more likely in a corporate system, fully documenting everything is a huge hurdle by itself. So your best bet is to gather everyone that is related to a change and let them evaluate its impact.
nunez 3mo ago
Before the PR is cut, before the PR is sent to review (so that time isn't wasted on low-valuw bugs and problems --- engineers are expensive!), pre-merge and pre-release.

abtinf 3mo ago

[flagged]

abirch 3mo ago

Complexity is caused by Dependencies and Obscurities ~ John Ousterhout Looks like you had both.

thomascgalvin 3mo ago

> Our entire small team thumbs up a PR before it's merged unless there's a big rush on it, and this gives everyone on the team a rough idea of the state of the codebase at any given time. There's no being blindsided like "this whole system I depend on is gone" like I had happen at far more siloed places I've worked. How large is your team? Because I don't think that would scale beyond maybe five engineers I'm a huge proponent of automated testing, because that catches things like "this whole system I depend on is gone" even if the guy who depends on it isn't in the room I'm also a huge proponent of shared ownership of ... everything, really. It's natural for people to kind of own different pieces of a codebase, especially if it's a component they created, but that leads to silos and low bus counts. There shouldn't be one guy who owns one system that depends on one other component

koolba 3mo ago

> Our entire small team thumbs up a PR before it's merged unless there's a big rush on it, and this gives everyone on the team a rough idea of the state of the codebase at any given time. For non trivial or chore updates a second pair of eyes is always a good idea. But it’s not possible to scale out “everybody reads everything” to a large N. The problem is that nobody could keep up with that ad the reader when there are some huge number of things to read. That’s why we delegate, create docs, and have overview sessions.

Aurornis 3mo ago

> Our entire small team thumbs up a PR before it's merged unless there's a big rush on it This has been tried by a couple of my past managers. This feels great with a small team on a slow moving codebase. If you try to force it on a larger team or expect the codebase to move quickly then it turns into a performative game of skimming the code (if that) to click the thumbs up button so you can get back to your work. The end game was a situation where nobody was really reviewing code because everyone had their own work to do and they didn’t want to be the one person blocking important PRs, so everyone was clicking thumbs up.

cucumber3732842 3mo ago
This with a side helping of "when everyone is responsible nobody is" Then again, if you're dealing with an un-maintainable minefield of a code base then "fire the whole team, I dare you" might be what you're after.

Scubabear68 3mo ago

Based on your description, I don't think the process is working. You describe a shared database, micro services you aren't aware of using the same database, and a desire for everyone to have a rough idea of the state of the codebase. And things breaking unexpectedly being caught by code review. You have a bigger problem here of a system you can't reason about very well. The code review will help here, but I think you have bigger discoverability problems based on what I'm reading. As others have said, what is needed is automated testing that would catch this sort of problem automatically without needing a human in the loop saying "Oh, wait...". We still want/need humans in the loop, but they should not be the only safety net.

chrisra 3mo ago

Decisions by small groups should be the default. Others only need to be involved if the risk/consequences of failure are high. I started ignoring all PRs from our large team because we had a similar policy. My teammates can handle, they don't need me to check on each PR.

stronglikedan 3mo ago

> "this whole system I depend on is gone" This would be caught upstream in my org. If something was removed in a PR which was not documented and discussed with the entire team ahead of time, that would not be good for the submitter.