Thoughtful Code Review

Speaker 1: Hi. Uh is the microphone yes, microphone's doing the thing. So yes, hi, I'm Naomi. I'm on the core developer team. I also have a new job. I'm working at New America, which is a think tank in DC that uses Wagtail for their website. Thank you, Tom, for introducing us. And I'm doing data visualization, which is the most exciting thing ever to happen to me. But Um, I want to talk about doing code review in a thoughtful way that is nice to your coworkers because Or your open source contributors, because I used to be not very good at that and it was not very pleasant for my coworkers.

Speaker 1: And then I read a whole lot of things and I got better at it and working is much pleasanter now for me and everyone else. And one time Tom sent an email that said I was really good at communication And that was the other best thing ever had happened to me. So because I'd worked so hard on it. So first to clarify, because people get confused by this, I'm going to use positive and negative Feedback to mean whether you're saying the work is good or not good, and good and bad feedback to mean feedback that is done well or done poorly. Um oh and I'm gonna sort of briefly run through reviewing pull requests as a process and then

Speaker 1: um talk about sort of my approach, my toolkit for doing this. So at least in Wagtail we like to approve pull requests pretty liberally We uh maybe don't have time to do a lot of reviewing pull requests as much as we would like to, but um the bar is reasonably low. If it works, it makes things better, it doesn't make things worse, we should approve it. We shouldn't wait for it to be absolutely perfect and exactly the way any one person would do it. Though I will note that worse includes adding a lot of technical debt. So it's not just it's broken, but sometimes doing things not

Speaker 1: a helpful way does does indeed count as worse. I'm not saying that there's no such thing as bad unless it's broken or bad for the project to include. So the code review process, briefly, as I do it, or as I do it when I'm really on top of things. is first I want to get some context on the um on the pull request I want to look at Is there an issue that this pull request is addressing? What does the issue say? What have we been discussing? Does the pull request, and I'm going to recommend that in writing a pull request, you do this when possible Say, here are the things I did, here's a really easy way to test them.

Speaker 1: And Maybe even I'm going to have separated my changes into commits that are that are going to make it easier to figure out what's going on. So get context. The other piece of context that I think is worth paying attention to is who wrote this pull request is this um Like, is this Tom who wrote this pull request and definitely knows how Wagtail works? Is this like is this issue marked good first issue? This is someone out of sprint, they've never worked on Wagtail before. They may not have time to come back to this pull request. But they are very excited about doing something for the first time and you want to encourage that. Then we're going to check everything looks okay.

Speaker 1: I have a screenshot about the sorts of things that we're checking. This is just the um default um what comes up when you write an issue. Uh so check or when you write a pull request, the test passed, the code complies, etc. Um, I might skim the code while I'm doing this if it's a longer pull request. And the idea is that at each stage of this you might be finding that you're not going to keep going with this process because it needs more work or you're going to keep checking that everything's okay. And also checking that everything's okay also is was the approach used overall something that you think is a good idea? Or was this done in the most hacky way you have ever seen?

Speaker 1: And we should probably fix that before going into line-by-line review. Then you test that everything works. I'm usually doing front-end things, so I'm going to test it in a bunch of browsers. It's going to be miserable. Um and then you review the code line by line and then at least if you're me and how I think everyone should be doing it. You're going to look over your review as a whole. So here's um this is I made fake comments on Tebow's review. These are not real comments. I didn't actually take the time to review your polar crest yet, Tebow. Um but

Speaker 1: I just want to say that The start a review button on GitHub is pretty great rather than just making comments. This is the saddest thing about GitLab is it doesn't have this button Um also that it's not good at stepping through commit by commit. But um you start a review instead of adding a single comment, and then at the end, you can look at all your things And decide what you're doing and say, oh, were any of those comments actually irrelevant by the end? Or did I say, why did you do this? And then actually I didn't need to say that because it became clear. So, having rushed through how code review works,

Speaker 1: my primary tools for not being a jerk when giving people feedback. are to make sure I'm demonstrating appreciation for what they're doing, appreciation and respect and trust that they are good at what they're doing. Or at least that they are giving genuine effort, that they are not a bad actor here. That I want to focus on goals rather than prescriptive solutions. I get this especially in the context of design work, where like this is especially a thing for design feedback, but everything we do is really a type of design code. that you design your code, how you're going to accomplish things, so I think that that counts.

Speaker 1: And then I want to consider the overall impact. Which written feedback like code review, especially when you do it on GitHub like that, is really really nice for And I think that these tools apply to all sorts of giving feedback, not just to code review, and this all applies not just to reviewing code in an open source project like Wagtail where you really want to make sure that you're welcoming uh new people and giving them a good experience of interacting with Wagtail, but also applies just working with any coworkers, I highly recommend having someone to review your code if you're in a situation where you don't. Like I currently am the only developer

Speaker 1: um doing my work, but I actually have someone at Torchbox who reviews my changes to the website. I think I'm going to have someone else who's going to review my dataviz things and for me I find that really helpful in making sure that my Code is good. Yeah, so I just highly recommend always being in a situation where code review is a part of your life if you're writing code. Um So, demonstrating appreciation. Also, yes, I heavily stole from the Wagtail Space website for colors and things. Why do we want to do it? Giving feedback, code review as an example of it, is a really good opportunity to build community, to build relationships between different people working on these projects.

Speaker 1: To build people's confidence in their work when you say they have done a good job , which I will say that gaining confidence in my work has been probably the biggest thing that's helped me receive negative feedback. And I'm like, oh yeah, I'm like, okay, I'm good at what I do, also I need to fix this thing. Um, not I must be terrible at everything. Um And oh, which is basically my second point. If people know that you respect and trust their work, then it's easier and more pleasant for everyone when you're saying that things should be changed. And code review allows us to learn from each other in both directions. I learn things from people when I review their code.

Speaker 1: I learn things from people when they suggest that I could have done something a different way. And it's nice to say that it was nice to learn things from people. Um So how I go about this? Um mostly to start with this I just sort of Think about how I respect the person whose work I'm reviewing and I make sure anytime I'm saying something I'm communicating that. And that I don't say, well, that was like, how could you be so terrible as to do that? And I make sure that I'm keeping in mind that I I think this person makes good decisions, has good reasons for why they do things, and if they did something in a way

Speaker 1: that doesn't make sense to me, there's probably a reason Even if it doesn't end up being that they did the right thing for um to accomplish the goals of the project, they weren't like being stupid. That's a not a useful concept and b probably not true of them. Even if they just don't have experience in this, they're still trying. And even if they're not, it's more helpful to assume that they are. And framing changes as improvements instead of as fixes, basically saying, I value what you have done, and let's build on it rather than Let's fix that terrible thing you did. Some work some words for that. Improve, make stronger, better

Speaker 1: address a goal, which goals is the whole next section I want to make it clear that I put effort into reviewing because not receiving feedback is kind of as bad as receiving bad feedback. When you don't get feedback, you don't really know, like, am I actually doing a good job? Does anyone care what I do? Or is it just like, sure. Go to your thing, whatever. So if I don't have anything to say, I like to try to add a positive line comment somewhere. Saying that one was good, or saying when I tested this, everything was great. Like look, I took the time to test this. I cared to take the time to test this. Of course, there are tiny, tiny pull requests where you don't need to do as big a deal of that, but also it's partly about rapport.

Speaker 1: Like do you generally have comments? And if you don't have comments, it's just because you didn't this time And I like to point out when things are well done, these are probably real things I've said. Like a lot of thank you for doing this thing I personally care about. Like, yes, I really like when things are named that way. Thank you for doing that. And a lot of, you know, this is um You did this better than I might have, which I think uh works really well to balance Do you want to consider doing this a different way? Okay, and then this screenshot that I had before just shows an example of that, saying Good call making that a variable, Tebow.

Speaker 1: That one's an actual comment, by the way. Thank you. Good call making that a variable. Okay, focusing on goals. Why? It's more productive. You can more effectively improve the quality because rather than saying Change X to Y, you're saying let's do a better job of accomplishing this goal. And the person who wrote the code may have a better solution than change X to Y, but you won't really know if you just tell them what to do. And you give people more ownership of their work. Rather than saying, no, just do it my way, you say, here's the thing we need to work on, and

Speaker 1: but it's still your project to decide how we fix it. Um so how to say this? Um I tie requested this is just examples of It's like this is actually a thing that didn't work. I'm not just mad about this. Like when I tested it, I couldn't do this thing Or I think it could be better in this way. This is a thing where that context from looking at the issues and the what was said in the pull request is really helpful. Because you know what goals the person was trying to accomplish. This is also a thing that when you're writing a pull request, um, and especially a work-in-progress pull request, I think it's really helpful to say, here's what I was trying to accomplish, and for a work in progress, here's what I know I still need to work on.

Speaker 1: Here's what I know I still need to work on that I don't really need feedback on because I know it's bad and I know I'm gonna fix it. And here's what I'm still working on that I would like ideas on. Basically all of this, I think, if you think about what's helpful when you're reviewing, is helpful when you're asking for feedback. You're like, what would be the most helpful things for someone to provide me with when I'm giving feedback? Um and offering this one's a big thing that I'm working on that's hard. Um offering ideas and tools rather than prescribing them. Because with code review, especially if you're asking someone to change things, it can be really nice if you say and to make it easy for you

Speaker 1: Here's what you can change it to. But it can be sort of less nice if you say do this. So yeah, I'm working on saying like, oh, here's how I might do it Here's an option in case that's helpful. And also I like to provide links to documentation because you don't know if they know this. In an open source project, you can avoid making assumptions about what the person knows when you're doing that because you're like, oh, this is information for anyone reading this. Here's the documentation about this thing that might be useful, or because I was looking it up when I was looking at this, so I'm just gonna copy it here so it's convenient. Um yes

Speaker 1: And addressing high-level issues before details. I think this is really helpful both when you are reviewing and having and responding to feedback. If there's an issue that is um well like if tests aren't passing, if any of those things in that checklist of things your pull request should do aren't working. Those have to work. We're not going to root we're not going to approve it if those things aren't being followed. But also if there's something that's going to necessitate a change in direction, then It's not worth doing little comments about, you know, you have a comma in the wrong place if actually that whole bit of code might be rewritten. So you can just not do the line comments yet if something's all going to need to change.

Speaker 1: And then here's a big one. When you are like, why did they do this terrible thing? Or even when they didn't do a terrible thing and you just aren't really sure why this is done. It's like, can you tell me more about why you did this? Not this sucks. But I think it's actually important to also use this in situations where you don't think it sucks, to ask for more of the reasoning about why people did things. when it's good too so that it doesn't feel like well that's code for this is terrible. Um and also Um often when someone did something that definitely doesn't work, at least in your opinion

Speaker 1: Sometimes knowing how they got there helps address it and address it in a way that isn't just telling them they were wrong and gives you more empathy and appreciation for the work they were doing. So then looking at the overall impact. This is You can make all of your negative comments as thoughtfully as you want, but if it's a whole ton of negative comments and basically nothing positive, it's still not that fun to receive that review. So you want to make sure it's an overall positive message, and you also want to make sure that your feedback is useful isn't just like

Speaker 1: please make it better but here is a thing that we could accompl that uh a measurable thing we're trying to accomplish When you have done X, you will have succeeded in improving this. How to review your review? You're looking for this was good work, not This was a waste of time. Just putting yourself in the shoes of the person who has asked for that review. You want to avoid assumptions about knowledge level when possible. I know, I think particularly as a woman, it's hard for me. When people either assume that I know things that I don't or assume that I don't know anything, and I've gotten a lot better at

Speaker 1: being okay with that and these things, but it's still it's nice not to not to be making assumptions about w what the person knows because you never know when that's going to feel like a way of saying that you don't respect their work or the you don't respect their experience or you don't think that they belong if they don't know this thing or any of that. Though that one is hard to accomplish well, I think. And making sure that your requests are actionable and reasonable, which I think a big thing is requesting a lot of work. You want to think about are, is receiving my review telling someone they have to do a lot of work now? And is there any way that it doesn't have to be that way?

Speaker 1: Sometimes it just has to be that way. But is there anything where the only problem is that it's not how I would do it? And so some of these things I just say that the only that like I'm just offering information about how I would do it a different way. But sometimes I also remove those things because I did not need to say them and the balance of what I'm saying on the whole is Not as positive as it could be. Or is a lot of like, why do you do it this way? Why do you do it that way? Can you do it a different way, please? Um And then also just making clear which things are totally ignorable. So there's often a convention in code review, depending on who you work with, of saying that certain things are nitpicky.

Speaker 1: Um I will say that. I will say there's no need to change something when I'm writing a line comment. And I will also, in my overall review, which might be the next slide, yes, say this is the one where all the comments are fake. This is really great. The only thing that needs to be changed is X. Yeah, I don't think there are any layout bugs. That's that was that was did not test it. Um But like There may be other comments here, but the only one I really need you to address is this. Everything else is just like thoughts that you can take or leave. And I think that makes a difference both with setting the overall tone. This message is a really good place to say. This was really great work overall.

Speaker 1: This is like I really appreciate the work you've done and to have that be the takeaway message. I think ending on a positive note makes a really big difference. And it's also really useful as a checklist when someone's making changes that they can say, did I do the two things that the comment said I should do? And then they can do them. And that is the end. So I think I'm over time and probably cannot take comments. I don't really know what time is. Am I right on time? You're right on time. Okay. So probably not questions

Speaker 1: then unless they're quick, yeah.

Speaker 2: Questions. So I mean it so it seemed to me that I know like during talk it's really um but like it's There's kind of like a theoretical issue when you're not being negative to someone. Like one of them is like it seems like a lot of times things repeat the wrong way and there's nothing really wrong with them. They're just not your style and you definitely don't want to put that in your code. But I don't know, maybe in a project like WebTel you bas basically get good things and it's not such an issue. But um I mean there must be some times when you just think this isn't that good and so how do you kind of get in a different mindset or what you

Speaker 1: Yeah, I think my approach is to say it's not necessarily that this isn't good work. Like maybe this isn't a good solution for the problem we're solving here It's like this is I really appreciate the work you've done. This was really worthwhile. Unfortunately, it's not going to work for what we're doing.

Speaker 2: Just to localize it. You don't try and make yourself God, you just try and make yourself web town.

Speaker 1: Right. A lot of it is you were good. This doesn't work here And I think that's a lot of the thing about making things about goals too, that you are saying you did great. This goal. It's just it's just in this goal that there's a problem.

Speaker 2: That makes sense Okay, thanks.

Speaker 1: Yeah.

Speaker 3: I'm not coming to any buddy on my team, I want to make them perfectly clear. It's clear that they lazily did not follow the guidelines. It's like it's clear that they just wanted to get their They're one thing done and they just did it all rules. It's like

Speaker 4: sorry, Ryan.

Speaker 1: Right, so when someone clearly was just quick about it and didn't follow the rules, I would say there are two things. One is the empathy for it's not that they were lazy or stupid, it's that They were focused on other things. This was not their top priority. So I try to put myself in that mindset. Basically in general, when I think someone did something, I don't get, or that was I found inconsiderate to me. And like they were focused on other things That sucks for me, but like that's okay. They're allowed to be focused on other things. And then say, hey, this is a great start. Would you mind checking off these boxes that we require everyone check off? Thanks.

Speaker 1: I

Speaker 4: was wondering if you have a little bit more. You mentioned offhand how you have essentially some peers that review your code that you don't work directly with. So are you talking about like New America stuff that do you have them review as well? And I'm curious about working relationship that works because like we're a very small team of developers and we review each other's good but like still around asking other people review like

Speaker 1: So um we do have Torchbox working as contractors. So it's um the uh Kevin, the person who is doing code review for me um also is familiar with that code base. And I think I had initially thought actually that if there wasn't anyone, I would probably say, hey Harris, do you want to trade code reviews? Like let's just I mean you may have someone, but if you didn't, find someone where you say, what if we have an established thing where I review your code and you review my code and then we get familiar with each other's code bases and it works okay Thank you.