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.

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?

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...

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.

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

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.

If anything refactors should be behind feature flags *because* they are so disruptive.

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.

I mean you can have validation for the features you’ve written already behind the feature flag, while holding off on the stuff that doesn’t exist yet.

Feature flags don’t mean throwing the baby out with the bathwater.

Ship of theseus in practise.

Kind of.

Atomic commits are hard.

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

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?

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.

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.