undefined | Better HN

0 pointsbobthepanda1y ago0 comments

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.

0 comments

anonymoushn1y ago

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?

withinboredom1y ago

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

anonymoushn1y ago

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.

dkdbejwi3831y ago

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

2 more replies

moritzwarhier1y ago

Ship of theseus in practise.

Kind of.

Atomic commits are hard.

bobthepandaOP1y ago

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

ahepp1y ago

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?

bobthepandaOP1y ago

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.

j / k navigate · click thread line to collapse

0 comments

anonymoushn1y ago

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?

withinboredom1y ago

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

anonymoushn1y ago

dkdbejwi3831y ago

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

2 more replies

moritzwarhier1y ago

Ship of theseus in practise.

Kind of.

Atomic commits are hard.

bobthepandaOP1y ago

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

ahepp1y ago

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?

bobthepandaOP1y ago

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.

j / k navigate · click thread line to collapse