[ecshafer]

Good PR culture is definitely something that has to be built from the ground up, and supported top down. At Shopify, who I think has a really good PR culture we have a few things that I think help (beyond a good CICD, and static analysis tools):

1. PRs are supposed to wait for 2 acceptances, can be shipped with 1, and can be emergency shipped with 0. So the barrier is low, but the culture supports more. We are expected to get 2 reviewers from our team to okay.

2. Depending on the code project, we have to fill out a template for the PR, what is in it, what it changes, what to look for when we test the code, etc.

3. Some areas have code owners that might require an additional review from a specific team.

4. We are expected to check out, and test branches when we review them. So a quick read and LGTM is really discouraged outside of a few small cases.

I have seen a lot of places that do the blind PR acceptance, and its tough because without this really being enforced and encouraged that culture is hard to change.

[bobthepanda]

Also there is something to be said that code reviews also work well with code that is meant to be reviewed.

The worst kind of peer review happens on PRs that are thousands of lines because nobody wants to read all that and things will be missed. Where I have seen successful code review is where people break code into reviewable bits, and those individual reviews are so fast that they actually end up bring completed faster than if it had been one giant PR.

[anonymoushn]

How much additional time is needed to break a self-contained change that's the smallest it can reasonably be without breaking anything into a bunch of smaller changes though?

[withinboredom]

Like 10-15 minutes ....

    git co master
    git co my-branch -- .
    git add -up . # select changes relevant to first pr
    git commit
    git reset HEAD --hard
    # and again...

[anonymoushn]

The question was specifically about scenarios in which this approach wouldn't work, for example because your team doesn't want to approve PRs containing only dead code or because any subset of the change won't compile or won't preserve correct behavior without the others pieces.

[dkdbejwi383]

It helps to have the right tooling in place to ship "incomplete" work, e.g. feature flags so that you can ship a very light and not ready for end-users version of some feature, and continue to iterate on it in smaller PRs.

e.g. first pass adds a new screen and just dumps the output

second pass adds input fields and controls

next pass adds validation

then add animations

etc

[mewpmewp2]

It sounds so good in theory, but in practice:

1. Frequently old code needs to be touched or refactored. Feature flag would not be enough.

2. Even feature flag itself can be a risky addition, and might affect existing customer usage.

Most of the time old code does need to be touched, there really aren't those perfect new isolated features, at least in my experience.

[jappgar]

IMO this is a terrible approach, and why I hate the way feature-flags are used nowadays.

For example, I'm not approving anything without input validation (frontend or backend). I have no idea if you're actually going to add validation later before the fflag is removed. "Trust me bro" doesn't work for me.

[moritzwarhier]

Ship of theseus in practise.

Kind of.

Atomic commits are hard.

[bobthepanda]

This is so common that it is an architecture pattern: https://learn.microsoft.com/en-us/azure/architecture/pattern...

The original post describing it: https://martinfowler.com/bliki/StranglerFigApplication.html

[ahepp]

do you think this can be done at the level of commits in a PR, or is there a big advantage to making it multiple PRs?

[bobthepanda]

If each commit can be exposed to a separate review then that works.

The main issue is humans,

1. Do not want to read long things, slowing down approvals since no one wants to touch it

2. Cannot effectively review long things and something is more likely to slip past.

[mewpmewp2]

We have these things as well, but usually people treat these are bureaucratic obstacles and don't actually perform the steps. E.g. template is ignored, and reviewer doesn't check out, just LGTM and good to go. Few people actually take a more serious look.