Changes Requested

Posted on by in Development, Process

Each team has their own tolerance for what is and is not a reason to request changes on a PR and block it from being merged. This may be rooted in process, fairness, and expediency, and may be a company or team decision. Whatever your personal philosophy may be, the team as a whole and often the company has to come to some sort of consensus as to what constitutes a reason to block a PR. Over time, I’ve gotten a little more “block happy” and I’m going to talk about some of the advantages and pitfalls of blocking, and give you a mega list of reasons you might want to block a PR.

To block or not to block?

Everyone seeks approval in their life and someone requesting changes on your PR may, for various reasons, be an emotionally fraught experience. Certainly, the bright red interface, which is useful in warning you not to merge, is not exactly the friendliest thing to see when you’re working on a deadline. Plus, I’ll admit, occasionally I’ll feel incredibly proud of my work and then see “changes requested” and be like, “Noooooooooooo! My code is perfect! Why don’t you like me and trust me?” There’s definitely a team trust element I’ve found on some teams where they want people to feel trusted and charge developers with making responsible decisions and feel blocking a PR is antithetical to that goal.

I have nothing but admiration for philosophies around team trust. However, there are solid reasons to be explicit in requesting changes and several advantages to blocking PRs. At an organizational level, blocking PRs is a stronger guard against not letting bugs, technical debt, and security issues into your app and maintaining stability. On a team level, however, there are several reasons that requesting changes is helpful for communication as opposed to reviewing and not giving approval.

Communication-based reasons to use request changes / block:

  1. Clarity in communication. It sends a signal that the reviewer feels there are sufficient issues such that the PR should not be merged in its current state. In cases where anyone can approve, you’re clarifying that this is a big issue, not just a minor issue, and that can help with prioritization of what gets refactored. 
  2. Avoids approval limbo. Sometimes a developer will end up in “approval limbo” with a bunch of reviews but no approval and the poor developer unsure as to whether there is a blocking issue or just a bunch of helpful / unhelpful people with advice. If you actually block PRs, then what to do with code reviews without approval also becomes a lot clearer — ping the person and tell them they forgot to :+1: it, or read the review but understand you’re not awaiting their approval and need someone else’s. 
  3. No really, don’t merge it. The rules are more explicit for everyone, including the perhaps merge-happy folks that will attempt to merge without approval.

Some pitfalls to blocking

  1. General slowdown. If someone blocks a PR and then is unavailable, that can really create a general slowdown on the team and blocking, in general, can have the side effect of a bureaucratic slowdown. There are several ways to circumvent this, including to not block PRs if you’re going on vacation, you’re going into a long meeting, or you’re going to dive into a bunch of work and not come out the other side for a while. You can designate someone else to approve in your stead. You can also decide as a team that turn-around on code reviews and re-reviews for revisions is a high-priority and should be prioritized over writing code. 
  2. Lacking clarity in how to unblock. Sometimes blocking by itself is not sufficient for a developer to understand what they need to do to unblock it. Be sure to clarify what the blocking issue is, either by highlighting in the individual comments that this issue is the blocking issue, or in the original changes requested comment. I often do both, specifically using the :exclamation_point: icon in individual comments to highlight when I’m talking about a blocking issue.
  3. Blockers unfairly applied across different team members. Another pitfall is some people’s PRs are more likely to get blocked than others, even if the code quality is around the same. More senior people might be more trigger happy with their request changes button and early career people might be more reluctant to press that button on someone with more experience, especially if they don’t know if they’re right or if it’s a good reason to block. Like with everyone, perception and bias also play a role in how well a PR is reviewed and how much benefit of the doubt the coder is given. If only some people’s or one person’s code on a team is being regularly blocked and slowed down, it might be time for a check-in on process, perhaps in your retros.
  4. Blocking is subjective. There’s additionally a certain arbitrariness to what is and is not a blocking issue. Different developers have different tolerances for blocking or code quality in general. Like with everything involved with coding, there’s a lot of subjectivity. 
  5. Team trust is a prerequisite. When you’re reviewing PRs, you need trust to hear one another’s ideas and trust in each other’s work. In order to be able to give any sort of feedback on a team, you need to build goodwill and show that you’re seeing the good work they do through explicit encouraging comments in their PRs and in person. This goes double when requesting changes.

Remove ambiguity about what is and is not a blocking issue

If there is contention about what is and is not a blocking issue on a team, consider a meeting where you explicitly discuss the rules for blocking a PR and what might turn up in a code review that’s non-blocking that you may still want to comment on. Getting everyone on the same page and abiding by the same rules is a useful way to dissolve tension across a team. I also find this gist particularly useful when I write code reviews in clarifying what are style suggestions, clarifying questions, and blocking changes, and I encourage this as a team process if the folks on my team are game. I especially love the focus on how you need to highlight the positives in PRs, and this becomes even more critical on a team to foster goodwill if you do reach for that blocking button.

I created a mega list with some help from fellow software engineers at C5 of reasons that developers may or may not want to reject a PR from the super obvious “fails to compile”, meaning it would entirely break the app, to the more subjective “non-descriptive naming”. You can feel free to use this as a starting reference with your own team and can come up with your own list of reasons for blocking a PR. (I also use this now as a cheat sheet to see if I’m considering everything when approving a PR.)

Reasons You Might Block a PR

User-facing reasons

  • Breaks the app 
  • Fails to compile
  • Introduces a bug
  • Is likely to introduce a bug in the future
  • Is likely to allow a bug to be introduced in the future
  • Does not fully implement a story
  • Does not display correctly
  • Does not display correctly on all platforms/browsers
  • Adds in unnecessary functionality not approved. “You ain’t gonna need it.” (YAGNI)
  • PR does not consider migration from existing customer to new feature
  • Will slow down the user experience — i.e. N+1 query, non-performant code
  • Does not consider certain edge cases
  • Is not i18n compatible

Security

  • Introduces a security issue
  • Could introduce a security issue
  • Privacy concerns, such as storing personally identifiable information (PII) unnecessarily or without proper authority, not properly securing PII, and accidentally logging PII in the system

Legal

  • Don’t have permissions for the code
  • Code is legally contentious
  • Code is patented
  • Code doesn’t include necessary copyright
  • Not GDPR compliant

Process

  • Changeset is too large and needs to be decomposed into smaller PRs which could be considered independently
  • PR includes multiple features and/or bugs
  • PR fails to have accompanying issue in issue tracker
  • Lack of documentation
  • Wrong timing, needs other work to be done in the app first

Testing

  • Lack of tests 
  • Tests fail to test feature
  • Introduces a test that takes too much time for not enough value
  • Tests implemented incorrectly (e.g. implemented an integration test inside a unit test framework)
  • Breaks tests

Style & Communication

  • File or function is too long
  • Code is incorrect
  • Non-descriptive naming
  • Logic not clear from reading the PR
  • Functionality is already implemented elsewhere in codebase
  • Needs greater legibility
  • Unlikely that someone could fix a bug in this code due to legibility
  • Introduces too much technical debt
  • Is not DRY enough, needs a refactor
  • Too DRY, optimizing in advance and may be difficult to change later

Outside Technologies

  • Outside technology being introduced is not approved
  • Outside technology being introduced is too costly monetarily

Running a meeting about this topic

If you do want to come up with a list as a group, I would suggest the following format.

  1. Have everyone look at the above checklist or one you’ve made and mark what they feel are reasons to block a PR on their own. Be sure the starting checklist has issues that have come up.
  2. Hold a meeting and go one by one down the list and compare notes. 
  3. Create a list of all the agreed-upon checklist items first.
  4. Discuss one by one the areas of disagreement. I’ll note that my list has some areas of overlap by design as some people may think of the same thing in different ways. For instance, code quality may be a reason to block a PR for a developer, but not non-descriptive naming as they don’t see that as a big enough code quality issue.

If you have that one person that essentially feels “Never block PRs!” is the way to go, then perhaps start the discussion at an earlier point of what the use the blocking PRs might be, issues that come up when you don’t block PRs, issues that come up when you do block PRs, and move forward discussing those issues first.


Now hiring developers, designers, and product managers.
Apply now: www.carbonfive.com/careers