4 ms·

IANAM so grain of salt. > The "immediate" thumbs up on a PR This one bugs me too. Really? On a PR with 400 delta lines, you didn't need clarification on any

IANAM so grain of salt. > The "immediate" thumbs up on a PR This one bugs me too. Really? On a PR with 400 delta lines, you didn't need clarification on anything? I don't know what action to take here. > The PR that sits and languishes forever Get your team to agree to an PR review SLA on the order of a day or two. "Enforce" the SLA -- how you do this depends on your team and the situation, but it can help to just remind people that they collectively agreed to the SLA. Suggest reviewing PRs with the same cadence as checking email (~1-2x per day). > The chat room of silence when someone asks an architectural/design question Identify one or more "natural leaders" as mentioned in the article, and encourage them to check on the chat room periodically (email cadence?) to route questions to the most likely person. They should know who that person is because they're generally well-connected, and mentioning them by name creates social pressure for that person to respond.

ChemicalWarfare 10y ago
On the flip side - to properly review a 400 line pr would take hours unless you were involved in discussions with the developer along the way like let's say he's making changes to the api while you're working on the consuming client. I typically ask for the devs to push their code into the repo incrementally as long as the change doesn't break anything. That way you have manageable prs and if there's a need to go back to the previous version you have smaller deltas to deal with as well.
- crdoconnor 10y ago
  >I typically ask for the devs to push their code into the repo incrementally That's fine provided each of those PRs gets reviewed and merged almost immediately but if you don't do that then either the developer has to create a separate branch for each incremental improvement (horrible) or let them mount up into a monster pull request (also horrible). Some changes also kind of need to be large in order to not break anything (deep architectural changes).
  - ChemicalWarfare 10y ago
    Yea speed is key with incremental prs. Which is on me most of the time as a lead SA on the project. I encourage the devs to slack me directly or drop an @here note in one of the relevant channels. I do the same when submitting prs myself too.
geoka9 10y ago
You can also (soft) require people not to take on any new issues if there are any PRs outstanding.
StavrosK 10y ago
> On a PR with 400 delta lines, you didn't need clarification on anything? I don't know what action to take here. My thinking is that, if I approved a PR you wrote and it had a bug, I was as much to blame for the bug as you. That should help with frivolous approvals, but I've never had much of a problem with them, so take it with a grain of salt.
emeraldd 10y ago
A 1 line change can be just as dangerous as 400 line one. I'd argue that alot of the so called "small" changes are just as tricky to review as a big one simply because the context of what is being changed isn't really a part of the PR. (i.e the reviewer has to trace through the framing code to really understand what's going on.) The difficult part is keeping enough context floating to know when a deeper dive is required.

ChemicalWarfare 10y ago

On the flip side - to properly review a 400 line pr would take hours unless you were involved in discussions with the developer along the way like let's say he's making changes to the api while you're working on the consuming client. I typically ask for the devs to push their code into the repo incrementally as long as the change doesn't break anything. That way you have manageable prs and if there's a need to go back to the previous version you have smaller deltas to deal with as well.

crdoconnor 10y ago
>I typically ask for the devs to push their code into the repo incrementally That's fine provided each of those PRs gets reviewed and merged almost immediately but if you don't do that then either the developer has to create a separate branch for each incremental improvement (horrible) or let them mount up into a monster pull request (also horrible). Some changes also kind of need to be large in order to not break anything (deep architectural changes).
- ChemicalWarfare 10y ago
  Yea speed is key with incremental prs. Which is on me most of the time as a lead SA on the project. I encourage the devs to slack me directly or drop an @here note in one of the relevant channels. I do the same when submitting prs myself too.

geoka9 10y ago

You can also (soft) require people not to take on any new issues if there are any PRs outstanding.

StavrosK 10y ago

> On a PR with 400 delta lines, you didn't need clarification on anything? I don't know what action to take here. My thinking is that, if I approved a PR you wrote and it had a bug, I was as much to blame for the bug as you. That should help with frivolous approvals, but I've never had much of a problem with them, so take it with a grain of salt.

emeraldd 10y ago

A 1 line change can be just as dangerous as 400 line one. I'd argue that alot of the so called "small" changes are just as tricky to review as a big one simply because the context of what is being changed isn't really a part of the PR. (i.e the reviewer has to trace through the framing code to really understand what's going on.) The difficult part is keeping enough context floating to know when a deeper dive is required.