2

Variance in pull requests across levels of seniority?

Profile picture
Senior Software Engineer at Grab2 years ago
  1. How do pull requests and pull request comments look like with an increasing level of seniority?
  2. Are pull requests for senior engineers not expected to have any comments for improvements? If yes, then what kind of comments should be there?
  3. Also, have seen some principal engineers not sharing their pull requests as they are quite high in seniority and merging it directly. Do you recommend that? If not, who should review PRs for very senior engineers if the team does not have engineers at that level?
219
2

Discussion

(2 comments)
  • 2
    Profile picture
    Meta, Pinterest, Kosei
    2 years ago

    1/2. Code review can always have comments, but you're right that the nature of those comments will change with seniority.

    • With junior engineers, there may be more comments around "this code is more idiomatic, please update" or "have you tested this with an invalid input?".
    • With more senior engineers, there's generally going to be more trust that they've done the basics. However, this should not be assumed, and the best senior/principal engineers will have very clear explanations for what they're doing, detailed test plans, and they'll make it easy to follow along. The comments here may be more like "What are the benefits of this approach as opposed to the simpler method?" or "I didn't understand how these modules communicate - could you explain and/or update the description of the PR?"
    1. I definitely do NOT recommend merging directly into mainline. This makes it harder to follow the history of the changes and the test plans. For the principal engineers, there's still a ton of benefit to having more junior engineers review code:
    • it's a great learning/uplevel opportunity for the team
    • a good comment/question can really help the principal engineer update their code
  • 1
    Profile picture
    Robinhood, Meta, Course Hero, PayPal
    2 years ago

    Also, have seen some principal engineers not sharing their pull requests as they are quite high in seniority and merging it directly.

    This is a red flag to me - Just because you're very senior doesn't mean you're immune to feedback. Principal engineers make mistakes too, and more junior engineers can definitely leave salient feedback on PRs of engineers more senior than them. Staff and senior are below principal as well - I'm sure engineers of these level can chime in with something useful now and again on a principal engineer's PR.

    I would bring this up with your manager and see what they think. Pitch opening up these PRs as a learning experience for the team - I have found that simply reading the context section + test plan of a code review by a very senior engineer can be an educational, "level-up" experience for a more junior engineer (a lot of my mentees learned from me that way).