undefined | Better HN

0 pointsecshafer1y ago0 comments

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.

0 comments

bobthepanda1y ago

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.

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

1 more reply

moritzwarhier1y ago

Ship of theseus in practise.

Kind of.

Atomic commits are hard.

1 more reply

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?

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

mewpmewp21y ago

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.

j / k navigate · click thread line to collapse

0 pointsecshafer1y ago0 comments

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.

0 comments

bobthepanda1y ago

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

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

1 more reply

moritzwarhier1y ago

Ship of theseus in practise.

Kind of.

Atomic commits are hard.

1 more reply

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?

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

mewpmewp21y ago

j / k navigate · click thread line to collapse