Giving and Receiving Great Feedback through PRs

Do you struggle with PRs? Have you ever had to change code even though you disagreed with the change just to land the PR? Have you ever given feedback that would have improved the code only to get into a comment war? We’ll discuss how to give and receive feedback to extract maximum value from it and avoid all the communication problems that come with PRs. We’ll start with some thoughts about what PRs are intended to achieve and then first discuss how to give feedback that will be well received and result in improvements to the code followed by how to extract maximum value from feedback you receive without agreeing to suboptimal changes. Finally, we will look at a checklist for giving and receiving feedback you can use as you go through reviews both as an author and reviewer.

To set the scene, let’s think bout what the goal of a PR review could be to help us think about how to optimise going through the review process. The obvious answer is that it is a way to get approval to merge code into another branch, usually the branch that results in the code eventually making it to production. Whilst true, this this misses some crucial elements of a great review process. Framing the process as a hurdle to be cleared is part of where difficulties can start to arise. For example, viewing it simply as an approval process can lead to people rushing through reviews, pressuring reviewers for approvals or considering just the reputation or credibility of the PR initiator rather than looking at a proposed change and considering whether it is optimal. An example of problematic behaviour could be that senior team members have their reviews approved with little scrutiny whereas junior team members have a difficult time landing their changes. To consider how reviews can be improved, let’s look at some of the potential benefits of a great PR process:

• It provides a learning opportunity for the reviewer seeing examples for how to solve problems
• It can set and ensure standards for a project
• Leadership can identify areas of potential growth of the individuals involved in the review
• Bugs can be corrected before they go to production
• Can result in improvements to the implementation through feedback and ideas from others
• A feature can be directed to the most appropriate code base

You can see that only one or two of the items are similar to a gating process (the one related to standards and the other around whether the feature is appropriate for the project). Seeing PRs as a hurdle to finish something off can mean that your team misses the full benefits from a great review. Summarised in a few words, the purpose of the review process is to ensure the code that lands is optimal and appropriate for the project. Having looked at the purpose of the review process, let’s dive into more specific topics, starting with how to give feedback.

Let’s start with some general principles to keep in mind when giving feedback:

• Along with opportunities for improvements, also point out what you think is well done!
• It isn’t always necessary for the recipient to act on feedback although they should consider doing so
• Poorly expressed feedback can lead to the recipient becoming defensive and valuable feedback getting lost in the ensuing war of words
• Without intentional counteraction, we tend to err on the negative side of interpreting feedback
• Many of us connect our identity to the code we write and can interpret feedback on the output as a personal attack
• Some people need time to absorb feedback
• Many of the above examples are subconscious and not deliberate, many people don’t think they act in this way despite actually doing so (including myself before consciously improving my behaviour)
• The goal of providing feedback should be to achieve optimal outcomes (take this one to heart and challenge yourself whether your are living by it)

Let’s explore the above principles in the context of an example. I have been writing a few linters lately so let’s use an example of a class definition for a visitor that checks function definition nodes in an abstract syntax tree (which is a representation of source code Python uses to execute your code). Specifically, it checks that function names are lower case:

import ast


class Visitor(ast.NodeVisitor):
    """Visits AST nodes and check function names.

    Attrs:
        problems: All the problems that were encountered.
    """

    problems: list[tuple[int, int, str]]

    def __init__(self) -> None:
        """Construct."""
        self.problems = []

    def visit_FunctionDef(self, node: ast.FunctionDef) -> None:
        """Visit all FunctionDef nodes.

        Args:
            node: The FunctionDef node to check.
        """
        if node.name.lower() != node.name:
            self.problems.append((node.lineno, node.col_offset, "function names should be lower case"))

There are a few potential problems with this code. For example, ironically the visit_FunctionDef method uses CamelCase for part of its name. Also, Python has an inbuilt method str.islower() which would probably be preferable to writing a custom check like node.name.lower() != node.name. Let’s try to write feedback for the naming of the visit_FunctionDef method and consider how it could be interpreted and how the recipient might react.

Your name for the visit_FunctionDef method is wrong.

Expressing the feedback in this way could be interpreted to mean that the reviewer considers themselves an authority on naming methods in Python and that the reviewer believes the author has no idea about how to code in Python. The author is likely to react poorly as their only recourse is to disagree with the reviewer which they might be reluctant to do, especially if they are not as well established and secure in the team. The feedback also doesn’t suggest how the name of the function could be improved. Additionally, using “your”, “you” or something similar in the feedback can be interpreted to be a personal attack which may result in the author becoming defensive to protect their good name. After all, their reputation is important for securing ongoing employment!

Let’s consider an alternative way of expressing this feedback:

What is the reason naming this method visit_FunctionDef rather than visit_function_def please? Usually Python methods are named using snake_case: https://peps.python.org/pep-0008/#naming-conventions

What makes this feedback better? A key element is that the reviewer shows curiosity by phrasing the feedback as a question rather than a statement. This invites the author to share their thinking on why they named the function in this way, leaving the reviewer open to being wrong about this function being incorrectly named due not following the Python naming convention (which will turn out to be the case here). Most of us like to talk about the code we have written, especially if we have put a lot of thought into it, so the author is likely to respond positively to this feedback. Through the reasoning, the reviewer will also gain an understanding of the author’s general understanding of Python and software development, which can help calibrate the way the reviewer gives feedback (e.g., a junior developer may need more guidance). The author is likely to share that the parent class of Visitor, ast.NodeVisitor, expects the visit methods to use the pattern visit_<node type name> and the node type name will be in UpperCamelCase since that is the naming convention for types in Python. So it turns out that there is a good reason for not following the usual snake_case Python convention for method names in this case. (As a side note, I deliberately wrote “there is a good reason” rather than “the author had a good reason.” This is because a review should treat the code objectively rather than as the product of a person.)

Another key improvement is that the feedback asks about “the reason” rather than “your reason” which redirects the attention of the feedback to the code rather than the person who has written the code, reducing the chance of the author becoming defensive. The feedback also includes “please” which is great social lubricant. Another key element is that the feedback includes a reference to a standard that has been broadly adopted by the Python development community. This means that the feedback isn’t a disagreement between two people and instead referring to a broadly accepted naming convention for functions in Python.

Let’s use what we have discussed about giving feedback to address our second concern, the use of node.name.lower() != node.name rather than node.name.islower(). For example, we could write:

Would node.name.islower() work in this case? https://docs.python.org/3/library/stdtypes.html#str.islower

That is pretty good, it also phrases the feedback as a question, provides a link to the Python documentation and gives an example of how the line of code could be improved. To explore another scenario you could encounter in PRs, let’s say that the author responds by saying they prefer to write the check using node.name.lower() != node.name because it is clearer to them. Most developers would probably agree that using the str.islower would be preferred in this case and would probably insist that islower be used despite the personal preference of the author. This could lead to the author digging in and we get the comment war again. An alternative in these kinds of cases would be to give the author (and yourself) time to think rather than respond straight away. In many cases, people change their mind after contemplation for a time, even if that contemplation is happening subconsciously whilst doing something else. In some cases, they might even have the same idea themselves later and the change gets implemented in the following PR. That also means it can be ok to let a PR get merged even if it isn’t perfect yet and look for future opportunities to get the code improved.

In the past I have idealistically viewed these techniques of picking wording and phrasing carefully as unnecessary because other people should just engage with the feedback objectively even if it is expressed poorly. The lesson I learned is that this idealistic view will work well with some people and not with others, and may result n feedback not getting implemented despite its merits. We will next turn our attention to receiving feedback, which is where you need to be careful with how you react to be able to extract maximum value from the feedback you get, even if it is expressed poorly.

My goal when reading feedback is always to try to extract maximum value from the information and to control my reactions and emotions. That means I am generous with interpreting feedback. One useful technique is to read the feedback in the way I might have expressed it. For example, if I get something like the “your name for the visit_FunctionDef function is wrong,” when I’m reading it I’m interpreting it as “what is the reason naming this function visit_FunctionDef rather than visit_function_def please?” and respond as though the reviewer gave the feedback in that way. Nothing is lost and so much is gained because you can then explain the reasoning and get feedback on the reasoning. In this case I would also add a comment explaining why the function is named in that way to the code since any future readers will probably have similar feedback.

When responding to feedback it helps to explain your reasoning and get feedback on the reasoning as this will give more information to the reviewer to help them understand the code. When you get feedback it is also worth reconsidering the line of code, function or whatever the feedback is referring to and consider whether it can be improved in some way. Maybe not exactly in the way the feedback is suggesting although it is a useful starting point. This generalises in the following way. We know that completely ignoring feedback is suboptimal. Similarly, mindlessly accepting feedback shares many similar poor characteristics. It is a way to get through the PR process although doesn’t get all ht possible value from it and can even result in the code getting worse. In some cases feedback needs to be implemented exactly as given, in other cases you may need to modify the feedback and in others decide not to change the code at all beyond potentially a comment explaining the reasoning. In the latte case, it is usually best to get feedback on the reasoning.

To summarise what we have discussed for both giving and receiving feedback and also adding a few other items I couldn’t fit elsewhere, here is a checklist you can keep handy as you are giving and receiving feedback:

1. If you see something that you think is well done, point it out along with opportunities for improving the code!
2. Don’t avoid giving feedback out of fear of having a difficult conversation with the author of the code or other retaliation. Greatness comes from the courage to speak your mind thoughtfully when appropriate. It can be optimal to hold back feedback, for example, if the author is not ready to hear the feedback or it would unduly distract from something that is more important.
3. Avoid using the term “you”, “your” or a name of a person and instead focus on the code. Both when you are the author and reviewer, treat the code in the same way you would objectively review a car you are about to buy. That also means avoid using the term “I” (such as “I think so and so”) because using the term means that, in some ways, you are using your reputation or credibility to prop up what you are saying. It is better to just use the strength of the argument.
4. Use words such as “seems” or “could” generously because it leaves you open to receiving more information or being convinced otherwise.
5. Prompt others for their opinion after you give yours. This shows interest and may lead others to read what you have written more closely as well.
6. If you feel you are being attacked, redirect the focus back to the code rather than retaliate or closing yourself off to the feedback.
7. If you disagree with feedback, ask for the reasoning or the benefits and share the reasoning and cost and benefits of what you believe to be optimal. Avoid starting with “I disagree, here is the reason…” and instead put the reason first. If you start with the disagreement, some people don’t read beyond that or have already decided they will also disagree with you.
8. Carefully read and listen to what other people have to say. I have seen many cases of people violently agreeing with each other. Violently agreeing is where both people seem to be arguing and are not listening to each other well enough to spot that they are actually agreeing with each other (or near enough).
9. Keep in mind that you might be wrong as well.
10. Accept progress without necessarily demanding perfection. There is always the next PR were more progress can be made!
11. Avoid agreeing to make a change you disagree with just to land the change. Although it can be appropriate to agree to make changes if the reviewer is quite insistent, maybe they know something you don’t! In those cases, as always, it is worth doing your best to understand where they are coming from. Communication is not always easy.
12. Especially don’t make a change just to build or preserve a good relationship. Doing so can entrench poor coding practices and give manipulative people (you hopefully don’t interact with any) inappropriate sway over team coding practices. Good relationships are important and should not be tied to just doing what other people say. It is possible to both have a good relationship with someone and discuss the merits of different approaches to solving a problem. Adapting a quote from the negotiation book Getting to Yes, be soft on the person, hard on the code. (Incidentally, the Getting to Yes book includes many lessons relevant to pull requests which share many characteristics of a negotiation.)
13. Be generous when reading feedback and interpret the feedback with positive intent. Even if you are sure the other person has malicious intent, just react to the feedback as though they are trying to help you improve the code. That sometimes makes it so!
14. Watch your own emotions and reactions as you read feedback. Draw yourself back to the overarching goal of optimising the code if you feel yourself reacting defensively or with strong emotions.
15. Be patient with other people (and yourself). Even if you disagree in the moment, you or they might change their mind later, sometimes in unexpected ways! Getting into a fight in the moment can entrench views reducing the chance of a change of mind down the line.
16. When you get feedback, always reconsider the specifics in the feedback and also related functions, designs, choices and so on. Sometimes the reviewer subconsciously spotted a problem and wasn’t sure exactly what the problem was and pinned it on something else closely related to the underlying problem. As an example, feedback on the name of a function not being clear could actually indicate problems with the overall design which requires the function to do something that is difficult to name. Improving the design by breaking up the function may well resolve the naming problem along with improving the overall code as well.

As you can tell, this is a complex topic. We started with considering the overall purpose of a pull request being to ensure the code accepted into a project is optimal. We then considered how to give feedback with key lessons around focussing on the code, phrasing feedback as questions and giving people time to contemplate. This was followed by flipping to the other side on how to react to feedback. Key takeaways are to be generous and assume reviewers intend to help improve the code, watch your emotions and reactions to ensure you stay focussed on improving the code and not blindly adopting suboptimal feedback. Finally, we looked at a checklist you can use when giving and receiving feedback to ensure you get the most value out of the review process.