4

Code reviews and dependency in PRs in One diff, One Thesis

Profile picture
Anonymous User at Taro Communitya year ago

So I am trying to understand how code reviews are done in PRs and how dependency is handled when adhering to the One diff, One Thesis principle.

Assume we have the following 2 PRs:

  1. PR1
  2. PR2.

The dependency of these PRs is as the following:

PR1 <- PR2 (i.e PR2 is dependent on PR1 and hence PR2's review is also dependent on PR1)

Questions

  1. Let's suppose that I have submitted PR1 for review and that PR2 is now to be worked on. If I wish to start on PR2, do I need to wait for PR1 to be reviewed? If yes, then will I remain idle until PR1 is reviewed? If not, then do I not risk that I need to revise PR2 if the reviewer has blocking review comments on PR1?

  2. Should I in general, complete all PRs before asking for their review? Or should I ask for a review as soon as I complete a PR? What if I realize that PR1 has something missing once I start on PR2 and start using PR1 as a dependency? Do I then revise PR1 and ask the reviewer to review PR1 again?

  3. Are stacked pull requests a well-accepted way of handling such dependencies? Is it prevalent and accepted as a standard in the software industry?

    Sorry if this sounds like overthinking or irrelevant. I haven't seen a practical implementation of One diff, One thesis due to which I am unable to grasp it completely.

198
6

Discussion

(6 comments)
  • 1
    Profile picture
    Senior Software Engineer at IBM
    a year ago

    This makes sense and is partially an outcome of being in a world where we keep things small and maintainable, but need to map out ideas into multiple different stories and work with others to get them complete. When I work on PRs like this, I would put the first one out for review and leave it in the feedback column and then branch off of it and get started on part 2 while the first one is still being worked on. Doing something like this, I can complete an epic's worth of work in about a week, but still have the review process take ~1 month total by the time people come in, leave comments, we have the back and forth ideas and more. It can be frustrating, but as long as everyone in the team is cool with it, you shouldn't have a problem. When the review finally gets merged, then just pull down main and merge it into the new feature branch. Ok, you will have to solve a lot of merge conflicts, but that at most should take 30 mins - 1hr. Some people might take longer since they can get intimidated by the diffs. Just focus on processing the work and keeping everything small. Your coworkers will thank you!

  • 0
    Profile picture
    Anonymous User [OP]
    Taro Community
    a year ago

    @Brad Messer thanks for the insightful answer. So does your answer imply that stacked pull requests are indeed a standard in the software industry? Also, your epic example is really relevant to my problem. Let's say that the epic can be broken into 10 small PRs. And let's say that each one is dependent on the last one, i.e. PR1 <- PR2 ... <- PR10. Is this dependency accepted or would you want to minimize the dependencies like Pr1 <- PR3 s.t. PR2 is not dependent on PR1, etc.

  • 1
    Profile picture
    Senior Software Engineer at IBM
    a year ago

    I'd say it's on a person to person basis really. I just like to move fast and get stuff done and sitting around while waiting for others to answer in the good ole stop-go fashion of the corporate world is a great way to not make deadlines achievable at all. Adding in the I've got other things to do, work that really takes not long to do without comments comes out to being 1-1.5 months before all the comments have been addressed even though I did my part in about 1 week.

    That being said, I always tackle it so that PR1 becomes the basis for PR2 and so on as shown in the article and will merge PR1 into the main branch or target branch instead of doing the backwards cascading integration into PR1. Keep an eye on what is happening around you though and note what the most important work you should be focusing on is. I used this to great effect while in a development job, but got into a SRE job that just doesn't fit my personality and has me using my time poorly relative to coworkers. It is all about context.

  • 1
    Profile picture
    Senior Software Engineer [L5] at Google
    a year ago

    Brad did a great job already, but here's some more specific answers to your awesome questions.

    Let's suppose that I have submitted PR1 for review and that PR2 is now to be worked on. If I wish to start on PR2, do I need to wait for PR1 to be reviewed? If yes, then will I remain idle until PR1 is reviewed? If not, then do I not risk that I need to revise PR2 if the reviewer has blocking review comments on PR1?

    No, don't wait.

    Yes, you do risk having to rebase + revise your dependent PRs when you practice this method. This is typically a small amount of work, assuming you have already vetted your approach with your reviewers. Generally, blocking design changes during code reviews are extremely painful, since your subsequent PRs depend on the first parts of your chain. Watch this video (and the rest of the code review series) if you are not sure what this means.

    Should I in general, complete all PRs before asking for their review? Or should I ask for a review as soon as I complete a PR? What if I realize that PR1 has something missing once I start on PR2 and start using PR1 as a dependency? Do I then revise PR1 and ask the reviewer to review PR1 again?

    In general, send PRs as soon as they are ready (and ready is not the same as the code being "functional" - this same video covers this). If you have modifications to PR1 that is needed later, can you make this modification a PR on its own? That way, PR1 can be submitted without blocking for additional review.

    I find myself doing this all the time. I would plan a feature to be split into 3-4 PRs, but I end up with around 6-7, because there's sometimes suggestion of followups that I haven't considered that I work on and send for review later.

    Are stacked pull requests a well-accepted way of handling such dependencies? Is it prevalent and accepted as a standard in the software industry?

    Yes, I would say this is the prevalent best practice. Small CLs are not a panacea, but it's one of the most proven best practices in the industry. From what I can tell, almost all of the teams in big tech attempt to encourage this.

    One thing I'll add is that you do want to keep the amount of PRs you have to in concurrent reviews small, likely to a maximum of 3. Reviewers do have trouble keeping track of longer chains.

    So even developing a single feature, find a way to divide it such that not all changes needs to be in a single chain - as if you are dividing a single project for multiple developers to work on simultaneously. This will not only help you keep the number of concurrent reviews in a single chain small, it will also pave the way for you to handle bigger, more impactful projects with multiple developers.

  • 0
    Profile picture
    Anonymous User [OP]
    Taro Community
    a year ago

    @Kuan Peng Brilliant. Thanks for the comprehensive answer and great tips (small chains of PR etc).

  • 0
    Profile picture
    Senior Software Engineer at IBM
    a year ago

    @Kuan Peng Thanks for verifying just how much this has been shown and spread throughout industry. Some of the teams I were on were operated differently and this definitely shows how much I really enjoy small PRs. Thanks for sharing!