Taro Logo
0

What’s your tactic for reviewing a large PR?

Profile picture
Anonymous User at Taro Communitya year ago

When a colleague sends me a pretty large PR to review, I usually have no idea what’s going on. I know it’s not expected to pull their branch down but how do you usually go through it and add constructive feedback? I usually just wait until someone else has approved it (after their given feedback is incorporated) and then I hit approve too 😬

156
3

Discussion

(3 comments)
  • 3
    Profile picture
    Tech Lead @ Robinhood, Meta, Course Hero
    a year ago

    If the diff is very large (>500 lines), I recommend that they break up the PR into smaller ones to follow the concept of "One Diff, One Thesis". If you're going to do this though, make sure to suggest the collection it can be broken down into (e.g. "Let's isolate the data model into a separate PR and have a dedicated PR for the API layer"). If you don't do this, your feedback isn't very productive and will seem like just a raw complaint.

    For general advice on reviewing PRs you aren't familiar with, I recommend these other Taro discussions:

    I usually just wait until someone else has approved it (after their given feedback is incorporated) and then I hit approve too 😬

    Hehe, I like the positive energy here, but if you aren't adding feedback on top of the initial reviewer, this doesn't feel like the best use of your time.

  • 2
    Profile picture
    Senior Software Engineer [L5]
    a year ago

    It will be your responsibility to handle the code in the future, so I would be proactive now to make sure the code meets a certain expectation before it gets merged in. I've seen cases where hard-to-understand code gets merged in, and it ends up creating a situation where nobody on the team can understand the code, and no one wants to touch it. Everyone ends up treating it like a magic box. Then, when an incident happens, you are forced to dig into that code under a high-pressure situation.

    I would recommend trying to make an attempt to understand the code, then I would talk to the PR author about the code to make sure you are both aligned. Then, I would ask the author to break down the pieces of code that are the most confusing. I would also make sure that there are clear tests written to handle all of the edge cases for that code.

  • 1
    Profile picture
    a year ago

    When a PR is dense and intimidating, I have found it helpful to set up a brief team meeting and have the author talk through it at a high level and answer questions, before submitting our individual reviews.

    Ps. I actually enjoy pulling their code to walk through specific areas myself