Giving and Receiving Great Feedback through PRs

Speaker 1 [00:03]

Thank you for the introduction. Yes, like Jürgen, I work for Canonical all the way in Australia, but I'm here for a company event anyway, so I thought I would drop by as well. As Jürgen mentioned, Canonical is hiring all over the world, so you can join us from, I think we might be in 70 different countries, and people move between countries all the time. So it's a great company to come and work for and you work with some of the best people in the world that are having a great impact on the technology area. So what I'm going to talk to you about today is about giving and receiving great feedback through PRs. This is a somewhat, I'm passionate about this topic. One of the reasons is that when I first joined a technology company and went through the first pull request process, I had an experience where I didn't necessarily feel like I was sort of encouraged and it was a good experience to go through. And part of what I'm trying to do here is to help improve that experience for everyone and to help make it so that pull requests are sort of a great asset to you and your team. So we'll start off talking a little bit about what the objective of a pull request is, then we'll look a little bit into how to give feedback, and we'll go through a few examples, and then also how to receive feedback, and then there's a checklist at the end that is also available on my blog, which I'll have a link to at the end that you can go to and sort of refer back to. By the way, the images are intended to convey some of the feeling of what I'm trying to say and they are AI generated. The first topic is about the objective of a PR and this is sometimes where the problems can start depending on how people approach or think about PR. One of the ways that people think about it is that it's just the last hurdle to get to the finish line and I really want to close that ticket, I've been working on it for two weeks and it's Friday and I want to feel that I've accomplished something. Having this attitude towards PR, seeing it as a hurdle, can mean that a lot of the benefits that can come through a PR are missed. Another way to think about what you can get out of a pull request is the objective is to make sure that the code is optimal and appropriate for the project. So it might be that a particular contribution belongs better somewhere else. Some of the benefits of going through a pull request and having it well done in a team is that it's a learning opportunity not only for the person that's writing the code who's getting feedback but also for the other people who are reading somebody else's code and see an example of how to implement, how to solve a problem. It's also a great way to set a standard for a project and sort of people get to know going through the pull request the kind of feedback that you get and that is a way to calibrate the kind of contributions that you get as well. And of course, it's also an opportunity to spot any bugs, any sort of improvements that you can make. Okay, so now we have a bit of an understanding of what a healthy objective for a pull request process can look like. And then we will go through, next we'll go through an example of how to give feedback and some of, we'll sort of see some bad feedback and some good feedback and we'll characterize why some is good and some is not so good. Okay, so the first thing to think about when you are giving feedback on a pull request is why are you giving feedback, right? And the intent sort of shines through the words that you write and how you say it, right? And one of the important things to keep in mind is to make sure that the intent, the true intent behind what you're doing is to actually improve the code and to add value to it rather than use it as a way to block certain things or something like that. And there are a few things that are worth it to keep in mind when you are giving feedback to help sort of set or calibrate how you might respond to comments or what you expect to get out of it. So one of the key things is that the person receiving the feedback or the comments or whatever on a pull request doesn't necessarily have to act on it, right? So it's just a trigger point for a conversation to think about whether or not something needs to change. The other thing that's important to remember, and I'm sure you have gone through this, I've gone through this, depending on how the feedback is given can mean that it gets acted on and it's a good experience for the people involved or it can also be the start of a bit of a comment war, that sort of thing. And the third thing, which is really important to keep in mind, is that we're all very passionate about what we do, the code that we write and so on, so it can be that people link their identity to the code that they write so when you give feedback keep in mind that it might be perceived or people might get defensive about it and that's an important part of when you're framing it is to try and minimize that perception okay so now we'll go through an example so the code that i've could have thrown up any code but this is I write a few linters if you're interested you can ask me about that they're open source but this is an example of a linter that's not a real linter but one that checks whether or not function names are lowercase right and what you can see there are two problems that you can probably spot quite quickly the first one is that on line six the name of the function ironically is not lowercase of the linter that's supposedly checking for that and on line seven there's a kind of interesting way to check whether or not something is actually lowercase right there's other ways in Python that are probably easier to do. So let's look at line six to start with so how might we let someone know about our feelings of function dev so this is kind of an example of poor feedback right so I could say your Your name for visit function death method is wrong. You probably have never said it like that, but let's pick apart why this is a poor way of framing the problem. The first thing is that you're starting with, this is starting with your, so you're specifically addressing it at the person who wrote the code rather than focusing on the code itself, and then it can be very highly likely to translate to a perception as an attack on the person rather than sort of focusing on the code. The other thing is that it's not clear specifically what about the function the name is wrong. It's just saying that it's wrong. So it's not clear what might be a way forward here. OK, so let's look at another way that this feedback could be framed, which is more constructive. So what this person is saying, what is the reason for naming this method visit function def rather than visit function def in snake case, and then it goes on to say usually Python methods are named using snake case, and then there's a reference here, right? So some of the key ways that this is better is that it's addressing the code, it's not addressing the person who wrote the code, so it's talking about why this function name was named in a certain way. It's giving an alternative way, which the reviewer might think is an improvement. And it's also providing a standard, which it's not just an opinion. Here's an accepted standard in the coding community of how to name functions. So that helps to make sure that there's more credibility for the feedback. So I'll just give you the resolution of this. The way that the AST.nodeVisitor works, if you're not familiar with it, is that actually the visitor functions have to have a visit underscore followed by the name of the node that they're visiting. And that's why this function was named in this way. So it's actually correct. So in the first case, it might take a really long time to get to that answer. But in the second case, the person or the author of the code is likely to respond quite well to that question and explain that that's how the framework works. So let's look at one more example how we could give feedback about the line 7 there for checking whether or not the function name is lowercase. So in this case, this person is saying, would node.name.isLower work in this case, which is, of course, an inbuilt function? So in this case, we're giving an example of how this could be improved. And it's still a question in terms of whether it would be better. So it's inviting some engagement and some further conversation. And this is also an opportunity to give another example. So maybe this person comes from a different programming language where they're used to writing this in this way. So something to keep in mind as well is that there's a time and place for feedback. And there's always an export request where things can be changed or refactored, small refactored, and that sort of stuff. there's a balance as to how far you might want to push. Now, this is a pretty simple change, so it could be worthwhile to sort of pursue it, but, you know, in other examples, it could be that you park a conversation and then people come back to it, maybe with a fresh set of eyes a few weeks later or something like that. Okay, so now we have seen a couple of examples for how to give feedback, and certainly I had this feeling when I was, you know, a little bit further on in my time as a coder, like, is this really necessary? Do we need to do this? You know, I give feedback, and people should read it and, you know, think about whether it's an improvement. And the conclusion that I've come to is that, pragmatically, if you are careful with how you word things and how you frame conversations, it's way more likely that you will get the outcome or the intent that you're trying to seek, which is to get to improve the code. So, it's over many years of observing pull requests, giving feedback on pull requests myself, and receiving feedback on pull requests, and paying attention to how I react to how things are framed, I found that, you know, these kind of techniques, they help the team dynamics, and it also improves the ultimate code quality outcomes. Okay. So, we've spoken about giving feedback. There's also the other side, which is when you are the author of the code. In that case, you can't control how you will receive feedback, but you can control how you react to the feedback. So the mindset that I usually take is to try and get as much value out of it as possible, similar to these miners who are trying to get gold out of the ground. And how do I do that? Well, the first thing is how I set my mind or how I interpret what is being said in the comments is to assume that the other person is trying to be helpful even if I feel like they're not. Just assume it and actually in many cases, as you start engaging with the person, asking questions, providing alternatives and providing reasoning, then sort of the conversation opens up and you get to a good outcome. The other thing is that it's always a good idea to respond with reasoning and to provide reasoning before you disagree with someone, like to say, okay, here's why I was sort of doing this in this way, what do you think? And the other thing to keep in mind is that blindly adopting feedback that you receive is similar to completely rejecting it, because one of the benefits of feedback is that you're essentially getting a second opinion and you can weigh and think about which one might be accurate or get sort of information out of it but if you just blindly adopt what you receive as feedback then you're just doing what the other person would have done right so they're essentially coding through you and all your knowledge is sort of lost in that case. Okay so we've seen both sides of it in the last time until question time I'll share a bit of a checklist and like I said there is I'll share my blog at the end where you can find this checklist so it can be a helpful reference for you and your team to refer to as you're going through pull requests okay so the first one here on the checklist is that you know we naturally are inclined to when we see something that can be improved we're likely to sort of write something about it it's also worthwhile to recognize when you see something which is a really good implementation or a novel implementation or something that sort of is positive about it to call it out that also helps frame the conversation and that it's not just sort of you know there's lots of problems but there's also really good things being contributed here. The other thing is that it can be that you feel some apprehension, certainly in my early days, early years of a developer, I felt some apprehension about giving feedback and sometimes people say, oh, I don't have as much experience as the other person or there's something else that's going on. It's worthwhile to overcome that and you're not always going to be right when you give feedback. It'll be a good learning opportunity regardless. And something to consider as well, though, is it can be reasonable not to give certain pieces of feedback because maybe the time is wrong or maybe, you know, there's something else that's going on, like we really have to get this release done or something like that, and it's worthwhile not to implement certain things. The other thing, which I shared a little bit earlier, but I'll just reinforce, it's important not to use sort of you, your or I because that can drive the conversation towards the people involved rather than the code and then you get into all the problems of identity and people wanting to make sure to defend their name and that sort of stuff so it's always good to focus on the code and address the code rather than address the people involved the other thing that's worthwhile is to use words that sort of signal that you're still open to change your mind or that you might not have all the information, so things like seems or could are good words to use, especially if you're not 100% sure about certain things. As the conversation goes on, sometimes I become more assertive when it's obvious what the appropriate approach is, but certainly early on it's good to use sort of, there seems to be an opportunity for improvement here. What do you think? Those sorts of things. Okay. The other thing that's worthwhile is to prompt other people for their opinion when you've given your opinion because it shows that you're interested in what the other person has to say and then they are more likely to be interested to sort of read what you've written and be interested in what you have to say. The next point here is if there is some sort of focus on the people involved, it's always good to redirect to the code and avoid retaliation. It's worthwhile if that's happening in your team to maybe approach the leadership and try and improve the situation. In the moment, it can be helpful to just focus on the code and on the outcome that you're trying to achieve. When you sort of give, you disagree with something, it's always good to start with the reason. If you start with I disagree with you and here's the reason, then the other person is sort of to get to or inclined to also disagree with you. And if you start with a reason, then the other person is probably going to read the reason and then you can discuss that reason rather than just keep disagreeing with each other. It's important to read and listen to what is actually being said. I've seen many cases where people are disagreeing with each other but they haven't realized. So that violent disagreement is not as in physically but as in like people are very sort of arguing but they're not arguing in reality. So it's always important to kind of keep listening to what the other person has to say and also make sure that you're discussing the same topic or you're not sort of talking past each other. The other thing that's always, regardless of how senior you are, always keep in mind that you might be wrong as well. That's a very healthy thing because sometimes you think somebody else is wrong, well, flip side is that you and I are sometimes wrong as well. Sometimes it's worthwhile to accept progress over perfection. That way at least you make some progress and then you can keep making progress and iterate towards having really good contributions. The other thing that I sometimes see is that people want to land their change so they accept the feedback and implement it even if they disagree with the feedback so it's important to sort of talk that out and figure out what the optimal approach is rather than just make the change so that you can land the pull requests. The other thing that is important is that you shouldn't feel like implementing feedback is important to maintain a relationship. Those should be independent. You should have a good working relationship anyway and then resolve the technical issues or the real issues with the code and use that healthy relationship. If somebody makes it sort of a condition that you implement the feedback that you haven't maintain a good relationship, that's kind of unhealthy and it's worthwhile to try and address that, especially with the leadership. Okay, so the last four items and then we can go to some questions. I shared that before, it's always good to assume helpful intent, even if you know that or you're certain that it isn't. That can be a great way to turn around, turn the situation around and actually get to the point where everyone involved has helpful intent. I shared before that I actively observe how I'm responding to what I'm reading. So this is a really good skill to develop and not just sort of directly engage in the emotion. Like if you feel, oh, this person is attacking me, then you should still have a choice about what you do about it. You don't have to sort of go to your first instinct and go for it. This lesson took me a few years to learn. Sometimes when someone disagrees with you, you keep talking and you try and convince them and then the viewpoints get more and more entrenched. One of the things that helps is to give people time to sort of process what is being said and for them to think about it and then the really good people to work with, they actually sort of advance the thinking in the background and when you come back to it a few hours later, a few days later, you might be surprised by how much progress both of you have made, if you give each other time, and that includes yourself. Sometimes in the moment you disagree, and then you think, oh, okay, maybe I was wrong as well, or I can advance my thinking in a certain way. And the last point here is that when you receive feedback, it's important not to just focus specifically on the feedback that you receive, but also sort of think about, okay, you know, I got some feedback on the naming of this argument, but maybe there's some broader issue that, or some broader improvements that we can make put to the design or something like that okay that's all I wanted to share so that's the JDK Anderson comm is my blog so if you go there you can there's a link for the Python posts that I've written and one of them is titled the same as this talks and if you go there at the bottom I have all of the the checklist that I shared before. If you'd like to email me, that's my sort of professional email and then also my canonical email address, especially if you're interested in joining the team. And I'm also on foster.don as well. So yeah, thank you, and I think we have a bit of time for questions now.

Speaker 2 [22:02]

So it's just working oh, yeah, and so you got like many many questions I think it was 25 by now, so we won't be able to go through all of them But I will start like with the most like highly upvoted ones So the first one it's like How do you deal with PRs that are well intended but so bad that you don't know where to begin with the review?

Speaker 1 [22:26]

There are a few things that I can say about that. One of the great things that we have in our team is that we are really good at writing down what we expect of each other. We have a contributing guide, which sometimes we get feedback that is a little bit long. If you have that in your team, then people can look at what you expect from the contributions and rather than sort of you having to explain it over and over, people can read it and sort of understand what's expected. The other thing which can help a lot and we make use of this in our team as well is that we have automated checks. So one example is we have some guidelines about how to sort of put together a doc string in Python and then we have some tooling that checks that it's accurate, it's actually open source and you can find it on my blog as well. And that tool actually guides people to how to fix the problem. So those are some really good things. The other thing that is quite helpful is if you have a sort of newer person to the team or someone with a little bit less experience, and again, this is something that we do at Canonical as well in our team, is that we encourage people to write specs or designs before they start implementing something, and then that is a really good opportunity to give some early feedback so you can sort of intervene a little bit early. And then the other thing that you can do if it's sort of too late for that is that you can sit down and pair program with someone and, you know, try and have a mindset of, you know, there's good intentions here, let's try and help this person actually achieve the outcome, right, and that can be a really good learning opportunity for both parties involved.

Speaker 2 [24:19]

Okay, the next question is, how do you encourage a practice of thorough PR reviews when people just go, looks good to me, and approve all the time?

Speaker 1 [24:32]

Yes, this is something. So one of the ways that you can do that is by setting an example. I always take quite a bit of time to review pull requests and you know, I get a lot of thank-yous for that as well when people can tell that you've thought about it. And then the other thing that I find is that that gets reciprocated. So all the PRs that I submit, even some of them are very long sometimes and you know I get feedback about that too. I still get really good feedback and good engagement from people so it's in the moment it's difficult to develop but it's something that you can train in a team and sort of champion as well.

Speaker 2 [25:23]

The next question is, oh now it's changed, in open source it seems that PRs using explicitly polite language are frowned upon, whether straight to the point feedback, so where's the line to hostility?

Speaker 1 [25:42]

So, you can be straight to the point and avoid some of the pitfalls as well, right? So, all the examples that I discussed, I don't think they multiplied the contents in a big way. The examples that we looked at, they were still straight to the point. It was just framed in a way that encourages more conversation and more interaction with people or some more thoughts rather than outright rejection. and you know that for the other person it doesn't feel very good so those are some of the things to keep in mind is that you don't have to you know it doesn't mean that you have to waffle around a lot and talk beat around the bush it can still be to the point and still be sort of respectful and thinking about how the other person might react

Speaker 2 [26:35]

Is providing feedback in form of questions actually best practice? Doesn't it almost always implicitly convey sarcasm?

Speaker 1 [26:47]

I don't think so. I think it depends on how you frame the question, and it doesn't always have to be a question. That's one of the tools that you can use to encourage further conversation if it's warranted. In some cases it might not be warranted, and the other thing to keep in mind is that if you're going through a conversation at the start of it, especially a complex conversation, you kind of have to broaden the conversation a little bit and feel each other out and doing that through a question is a really good way of doing that. In other cases where there's a very clear standard that you've set, and I do this all the time as well, is that I say, hey, we have this standard. Should it be applied here? And then that's a fairly straightforward and people can go look at the standard and see if it applies or not. So you can frame feedback as questions and have it be sarcastic certainly possible and but I think it's also possible to frame questions to frame feedback as questions without them being sarcastic

Speaker 2 [27:55]

When giving a feedback with a link to the documentation, don't you fear it could be interpreted as read the fucking manual?

Speaker 1 [28:06]

I think that's in the eye of the beholder. It's worthwhile, especially if someone is new, they might not have read through every single point in the documentation, and it's worthwhile to give them some details about it. And the other thing that I tend to do is that I write a few blog posts about topics that I keep giving feedback about, and sometimes I point people at that as well. So if you have a well-documented and well-written source which addresses the issue at hand, it's much better to just refer to that and have people read that than you sort of manually rewriting it over and over and over. I mean, the other thing you can do is you can copy from the documentation if you really want to avoid that perception. But I think that if the way that you go through and give feedback and engage with people will shine through, and I certainly have never had feedback like that, like that, Sarah, you're just telling me to read the documentation, can't you give me the answer? I think people appreciate those things in many cases.

Speaker 2 [29:16]

So I would love to read all the like the next 15 questions, but I think here's the last one Do you have any ideas on out how to incentivize your co-workers to give feedback?

Speaker 1 [29:30]

Yes, so this is a, sometimes co-workers don't give as much feedback as you'd like them to. One of the things that you can do is after somebody has given you feedback, you can thank them for it, which encourages more feedback in the future. The other thing that we do is, you know, we have our leadership make sure that we recognize people that give really good feedback. So there are things that you can do in a team to sort of make it culturally more likely that people will give feedback. And depending on how people generally feel about the way that pull requests happen in your team will also influence whether or not they tend to engage with the process more or less.

Speaker 2 [30:15]

Okay, thank you