0

How to push back on tangential requests in code reviews?

Profile picture
Anonymous User at Taro Community2 years ago

I'm an E5 mobile engineer at a Big Tech company. Some engineers have a habit of leaving "drive-by comments" in code reviews that are only tangential to my PR. How should I push back on these?

Examples:

  • I created some snapshot tests using our existing 3rd party snapshot library. An engineer commented that we should change where the snapshot library stores the snapshot images. I felt that this was out-of-scope for my PR, so I told them to ask our mobile infra team instead. They did not follow up with the mobile infra team. Should the expectation be for me to drive those conversations with the mobile infra team instead of them?
  • An engineer requested that I abstract and mock how I log analytics events for my feature. I think there's little ROI because it's unlikely for that to change, but I did it anyway to appease them. When I later looked at that engineer's PRs, I discovered that they never abstract & mock how they log analytics events either! This seemed hypocritical and a waste of time due to the low ROI.
107
2

Discussion

(2 comments)
  • 4
    Profile picture
    Staff Eng @ Google, Ex-Meta SWE, Ex-Amazon SDM/SDE
    2 years ago

    The general answer is to ask if they block this critical work, or if they are nice-to-have and can be tackled separately in a dedicated CR instead of adding complexity to what you’re already doing since they are out-of-scope. Rarely does someone want to be on the hook for blocking something for superfluous reasons and have their name attached, so they will back off. You should create a ticket or ask them to in order to track the follow-up, but you don’t have to own it and they can help prioritize it if it’s really important.

    In terms of saying “some other team owns this library” I think it’s fine for you to open an issue against that team, link their comments, and CC the commenter. This shouldn’t take long, and doesn’t mean you own it. Pointing them to the actual person requesting helps, since it is really their ask.

    Leading by example is a good policy but I wouldn’t try to get out of things by showing that the requestor isn’t following the rules they espouse. This can only cause trouble. What you can do is ask if this should be a part of the team’s coding standards, and create an issue and put it into meeting notes for a team meeting to discuss if that is a standard you want to establish. If so, then this person is then more beholden to the standard. If not, you shouldn’t be required to be the only one doing more than required.

  • 1
    Profile picture
    Tech Lead @ Robinhood, Meta, Course Hero
    2 years ago

    An engineer commented that we should change where the snapshot library stores the snapshot images. I felt that this was out-of-scope for my PR, so I told them to ask our mobile infra team instead.

    I totally understand how this could feel frustrating, but I do feel like this "Why don't you do it?" response violates the concept of treating feedback as a gift and makes the entire situation very tenuous.

    I think a more constructive response would look something like this: "Thanks for the feedback! I totally agree that the snapshot images aren't in the best place. However, fixing that is a larger problem and out-of-scope for this diff. Here's the follow-up task I created for myself about this issue - In the meantime, can you accept the diff?"

    Here's what I like about this response:

    1. You immediately thank them for the feedback
    2. You create the follow-up task to speak with your actions and really make them feel heard
    3. You still flip the interaction back towards them, opening the gate to get your code approved
    4. You didn't really lose anything as "follow-up" tasks in Big Tech are lost to the backlog graveyard all the time 😂

    An engineer requested that I abstract and mock how I log analytics events for my feature. I think there's little ROI because it's unlikely for that to change, but I did it anyway to appease them. When I later looked at that engineer's PRs, I discovered that they never abstract & mock how they log analytics events either!

    As Lee mentioned, I wouldn't hold people to the standard that they have to be doing everything they suggest: Assuming good intent, it's very possible they know it's good but don't have the time to do the things themselves. This happens all the time in Big Tech.

    Similar to before, you can try suggesting it as a "fast follow" for a separate diff. If they reject that, then this is where "practice what you preach" comes into play more.