Pull Requests: Merging good practices into your project by Luca Bezerra

Speaker 1: Uh first of all a few disclaimers. Uh this is my first DjangoCon, so I'm very happy to be here. Also, I'd like to give kudos to the folks who are transcribing these talks because they're having a hard time and they're pretty accurate at it. And they might not like me very much because I'll have to speak quite quite fast due to the time constraints. But uh here you go. So also if you guys wanna Follow along this this talk, this bit. ly link over there, bit. ly slash DjangoCon19 has uh a blog post with the links for the presentations that all of us from uh Vinta are presenting here on DjangoCon. So here we go. As he said, I'm gonna be presenting a talk about pull requests, merging good practices into your into your project

Speaker 1: And the single reason I chose to have this image over there is that next time you see a door that says pull, you remember it's a pull request. So here we go. Uh I think I've kind of lost you here. Is it not quite? Okay As I as he mentioned, my name is Luca. I'm a full stack developer. I have a master's in computer science. I work mostly with Django and React, maybe some other Python frameworks as well. I work uh at Vinta. Vinta is a team of experts from Brazil. We do development and consulting for uh to build products the right way and uh if you have any questions about development if you want your project to be uh implemented or improved in some way

Speaker 1: just reach out to us there's our website over there uh if you want to check it out So going to the content itself, uh I imagine that most if not all of you have already dealt in some way with pull requests if you've heard about it, if you have to actually work on a pull request. And you probably have some ideas on how to do it, but I believe that for all of us, including you of course, there's always ways to get better at it, and some simple things can take you a long way in that process First of all, let's ask why should we review code? But instead of asking me, let's ask the people. There's a good article called The Ultimate Guide to Code Reviews by Codesi which I think it's quite a bold title by the way, in which they've surveyed uh 682 developers regarding seven

Speaker 1: different things. One of the things was on average what would they spend the most of their time on on a daily basis? So they said that approximately half of the time, 51% of the time they spent developing new features. And at the same time they spend 70% of their time working on technical debt and 28% of the time working on bug fixing. So that amounts for 45% of their time or almost the other half of their time. So they're half of the time working on new features, half of the time working on not new features. And that's the the that's because usually that that can be improved with uh good processes in code review, in code management uh fixing uh stuff before it breaks so you don't have to spend so much time on it. They were also asked uh

Speaker 1: what was the change in your development process that had the biggest impact to code quality. And of course they've mentioned a ton of things like tools testing, automation, but the thing that stood out the most was code reviews. So even though usually you hear people saying that reviewing code is kind of boring, people still think it's it's very valuable and brings a lot of improvement to the process. They were also asked if they will review code before or after deploying the code to production. I know it's a weird question for some of us. Some people uh review the code after the deploying to production. Most of the people review it before. A weird amount of people revealed it like both after and before the

Speaker 1: deploy to production. And uh similar number of people don't review code at all. And they're probably the ones responsible for this bug. Not know if you guys are familiar with it It was the iOS calculator bug where you sum up one plus two plus three and it would equal 24. Uh sorry, yeah They probably didn't test it very much, didn't review the code. So the the authors of that study came to a shocking conclusion. that they found out that doing code reviews before the deployment was far more beneficial than not doing code reviews at all. So yeah, my chalk right They also came to another conclusion which is interesting, that doing code reviews both before and after the deployment

Speaker 1: was kind of the same thing as just reviewing it bef after the deployment. And the reason for that, according to them, is that when you're reviewing something that you know somebody else is going to review it after, you don't pay as much attention as you sh as you would if you're if it was the only revision that you were doing that code. So for a few common errors regarding uh code review. Uh some people say that code review is a chore or uh they say they don't review code at all because it's not important or they don't have time for it or just skip the code review for the deadline. And uh I noticed like I've been uh in the position of saying I'll skip the code review due to the deadline. I think maybe a lot of us have been. And it's very tempting, but we gotta remember that when you're reviewing code, you're not only improving the quality of the process and the project that you're working in, but you're also learning.

Speaker 1: You're learning new ways to implement things, you're learning new things about the language or framework you work with. and uh you're learning a more about your project maybe areas of your project that you don't know yet uh and also while you're learning you're also getting paid to learn so it's like best best of both words right And for some best practices here, I might go through them in a bit of a topic-based fashion. So forgive me if it's too quick, it's just due to the time constraints One great good practice regarding code reviews is having pull request templates. This is because uh we as human beings tend to be lazy and uh our memory is very error-prone. So we can just trust ourselves or our our colleagues to remember everything that uh we are supposed to fill out when creating a pull request or having a template that says

Speaker 1: everything that the the the developer has to input in order for the reviewer to properly review it, it's very good. On GitHub you can just create a pull request, template, file, markdown file. on the root of your project. On GitLab you just create a markdown file inside the git lab. git lab folder and on Bitbucket you can go in the settings and configure it there You can also use a few other tips like status checks. Having status checks in your configured in your repository allows you for Allows you to have a continuous integration server, for example, which is gonna run your builds for every branch that you create. And it's gonna it won't allow the branch to be actually merged into master or whatever your main branch is until it has passed certain status checks like

Speaker 1: has it passed all the front end and back end tests has it passed all the linters etc uh adding guideline files as well is a great way to uh tell people how to contribute to your to your project. If it's an open source project, for example, and people don't know how should they like they've made a fix and they don't know how to submit it, how don't know how to create the pull request. Also creating approval or merge rules so as uh if you if someone is making a change in a specific file you can have for example uh Your repository configured in a way that unless someone else, some specific person approves that pull request, it cannot be merged for that specific file. So you can kind of protect specific files that are maybe too important for your project.

Speaker 1: You can also define push rules, for example, uh your uh pull request, the the branch of the pull request must begin with a Jira ticket number or something like that so you keep everything organized. Another thing that's very interesting is Git flow. Maybe some of you are already familiar with it. Git flow is a consistent architecture. that ensures that branches are always up to date. So the idea is that whenever you're gonna build a new feature you're gonna branch off the main the master branch so that uh you're pulling from the most up-to-date uh uh code and you're also not risking conflict because you uh everybody is pulling from that same set same branch Git flow also suggests that you create a separate branch for each feature.

Speaker 1: So the point is maybe you are working in a specific part of the project, like let's say a payment system of your product and you're fixing something in the logic and somebody says oh uh like the the the buy button in the in the payment system should be green instead of blue And uh could you also make that change and you say okay I'm I'm already working on that on that section of the code so I'll just change it. But if if you do that and maybe your main feature that you're working on uh gets like a lot of comments, a lot of change requests in the pull request. you're gonna actually hold off that uh that color change, which was a super small change that it could have deployed very quickly until you actually finish the whole feature. So you're actually holding back features that could have been being deployed Super quickly.

Speaker 1: So I think a great way to think about this is to remember the branches are cheap and they bring great flexibility. So you can Just push this push something else that's super small and it's gonna bring great value value while you work on stuff that's bigger and it's gonna require more time. Another great great tip here is to look out for the PR size. So if you look at this, if you look at this image right here, this is a PR with 71 files that have been changed. And uh well-defined issues when you're when you are creating the the issues in your sprint, for your sprint or for your backlog Uh if you have well-defined issues, usually they're well well broken down. Uh they tend to generate smaller PRs. So that's something that's not

Speaker 1: just up to the developer who's doing the change, but also for the per the people who are actually managing how the issues are gonna be distributed. So if you look at the as as I was mentioning, if you look at those 71 files Ain't nobody got time for that. So uh probably whoever's gonna uh probably whoever's gonna review that uh it's gonna have like a decrease in the review quality because If you have like five files to review in a pull request, that's okay, you're gonna spend your time thoroughly. But if you have 71 files, you're probably just gonna go through them like diagonally and uh hope it's of it's working fine. So uh you're gonna have a shorter attention span and a shorter attention span usually equals more bugs. And now uh an analogy that I like to make is that pull requests are kind of like a kitchen sink

Speaker 1: So I kind of need your input on this one now. If you look at these two images, which sink would you be more likely to put a new empty dish in? Like the full one or the empty one? Which one? The empty one? You would put the well that that was not the what the answer I was expecting for But okay. Oh and my analogy uh when you have something that's already full you just think like okay just another dish it's not gonna make any difference but if you're if you have this like this clean super nice looking sink If you put an empty dish there you're gonna be like, uh this is kinda, you know, this is not good. I like my my my sink to be clean. So Pull requests are kind of like that. If you start piling up pull requests that you got a review,

Speaker 1: you tend to just like the more you get you just don't care anymore. So uh an idea here is to make a habit. So maybe reserve a few days of your a few minutes of your day to review pull requests. So reasonably sized reasonably sized PRs usually shouldn't take much more than a few minutes of your day, like I don't know, 30 minutes. And also you could uh define days of the week to empty the queue So for example you can say that every Wednesday no matter what I'm gonna empty all the PRs that are my that are on my queue. I'm gonna review all of them. Even if it if it means that I'm just gonna work on reviewing PRs the whole day Because you gotta also remember that maybe you're not in the mood for that, but uh the more you wait to review PRs, the more features are gonna be delayed to be deployed to production.

Speaker 1: So you're actually holding back the project. Also try to always uh write clear commit messages so avoid the first one, the top one which just says fix PR comments and write maybe a paragraph which is gonna take you like 30 seconds to write and it makes it clear what what were the changes you did so maybe if you want to go back in time and revert back to some comment you know where the change was made was made what was done there And it really helps in the overall organization. So remember to talk about how and not just what you did. There's something here about positive and negative feedback. So here on the left side, for you guys I think it's the right side. Maybe some of you have seen this. This is an answer from Linus Torvalds. uh towards a guy who who created a PR on I think for the Linux kernel.

Speaker 1: And Linus is not the most polite person. He's known for that, but especially here And you gotta remember there's a human on the other side of the code review. And by the way, it's on between quotation marks because it's the name of an article. If you guys want to read it, it's very nice You should also remember that positive feedback doesn't mean to always agree with the person. You can give you can't disagree with the person giving a positive feedback When you do posit when you give positive feedback, people feel more inclined to expose ideas and because they know that they won't be judged, they won't be like uh may uh you know be judged uh and it per it brings the idea of the fail-fast uh way of thinking so uh people were gonna be more inclined to exposing their ideas and their ideas uh are

Speaker 1: gonna be put into trial, you're gonna find the errors much sooner or maybe find that the idea is not viable enough and they're gonna you're not gonna spend as much time like working on it to in order to like buy by the end of the of the project to find out that it doesn't work so you just spend a lot of time a lot of money in it. You just failed fast And of course, if you provide positive feedback, it perpetuates positive behavior. So whoever received that positive feedback is probably gonna give positive feedback to other people as well. When working with pull requests, there's usually two main roles, which is being the author or the requester, and the reviewer. As the author, I think the good I the best idea here is to always describe the issue in the pull request, not just point to the card or ticket.

Speaker 1: Because maybe sometimes you're just gonna put in the in the description, oh this fixes bug 3, 2, 4, 4 But the person who is reviewing that is gonna have to go click on that link, go to the issue, read the description, maybe read through a conversation, understand that maybe some of the requirements have changed and maybe the person is gonna review it and find something that he or she thinks it's wrong because the requirements actually changed throughout like some conversation. So if you just take some time to write what was the issue and what did you do, it makes everyone's life much easier As a reviewer, remember to always ask questions, don't make demands. So instead of just saying fix that thing, why don't you say like shouldn't it be like this? So it's a much more polite and nicer way to say it

Speaker 1: Also don't say like why is this variable doing nothing? You could also say it instead I don't see this variable being used. Maybe you should remove it because sometimes uh It's something that you might not be seeing that the person who wrote the code has already thought about that and you give the person the the opportunity to explain him or herself and maybe it's something that uh you didn't catch it at first, but then you think, oh yeah, that actually makes sense. Remember that you are not a linter to give imperative instructions. nor you're talking to an AI assistant. You are a human being talking to another one. So you wouldn't like to be talked in a negative way. So try to remember that when you're reviewing someone else's code. Another good practice is having a minimum of X approvals.

Speaker 1: So of course it depends on the team size, but the idea is to avoid scenarios where you say, oh I reviewed your code, you review mine, we both approve each other's code, and that's okay, let's go home. I do have kind of a real life example of that. I used to work at a company that had geographically separated teams and uh There was something happened with the folks at the other uh the other country, the other the team that was in the other country, where they would review their own code without passing it through us, which was not the kind of the policy that we had established And uh oh uh every now and then their code would come would come to us with bugs. So I don't know if the review process was not good enough or they're just skipping the review, I don't know, but uh that would happen. It would also break the team unity. We went we would end up

Speaker 1: uh like amongst ourselves saying oh those guys they always bring like they always ship uh broken code and it would create like a rivalry feeling between the teams even though we were a single team This practice also ensures that at least x plus one people know the code. So one being the person who wrote it and x being the number of people who review the code so that's actually good for the management as well so if the person who wrote the code is maybe in a sick day or something someone else has some knowledge about the code we can can review it and fix a bug or something like that Atlassian suggests that X is uh I'm sorry, Atlassian suggests that you assign reviewers like If you need like two a minimum of two approvals you assign 1.

Speaker 1: 5 times two, so three or two to two point five depending on the team size So you can actually speed up the like how how soon the pull request is going to be reviewed because you're going to have uh more people assigned to it. So whoever is head uh is available is gonna review that that pull request sooner. Another tip is to always include screenshots on UI or UX changes. The cliche says it's a picture is worth a thousand words. And also some changes are very small, some changes are like you're in the context, you can see them very easily, but someone who's reviewing doesn't know, so they might not be obvious. Uh using git blame usually we we write we we run git blame because we want to know who screwed up something in the code But in this case, it's also very useful to finding out who you should assign to review your code.

Speaker 1: Maybe you're new to that code base, maybe you don't know exactly that part and you don't know who to assign if you do git blame, probably the person who appears the most is probably the person who uh like recommended to to review the the pull request Also let the automated tools do the needy key observations. So don't be that person who keeps saying, oh, you forgot the semicolon here or you're riding camel case instead of snake case. Like have a process in your team where you have linters, you have a pre-commit routine that checks for that, so that you don't need to be the one doing it and being the annoying person that everybody hates. Uh remember to always teach and not just tell how to do things. So I'm gonna show an example about that, but uh when you're you're you're like suggesting a change in someone else's

Speaker 1: PR, uh teach why that's wrong, why that should be done differently. Don't just tell because otherwise a person won't actually learn And when something breaks after a code has been reviewed and deployed, remember to also share the fault because it has gone through your eyes, even though you were not the one who wrote it. but you've revealed it. So it's kind of your fault as well. We don't want to enforce the policy of pointing fingers at people, but the person who wrote the code is already going to be under pressure of being the the author of that. So help the person with that as well Quick tips about it. GitHub has uh some keywords of feature. So if you write in a PR for example close Pound 3244 it will actually close that issue when you merge the pull request so you don't have to worry about uh managing your issues

Speaker 1: You can also get permalinks to code snippets so it's easier to reference when you're reviewing code or mentioning to someone. There are some tools like OctoHint, Refiner Bitbucket, and Refiner GitHub. which are browser extensions for you to have syntax highlighting amongst other things that help you reviewing code on those platforms like you would see it in your text editor Uh review apps from Heroku and deploy previews from Netlify are some tools that they they allow you to actually deploy changes from a specific branch you've you've just created and are requesting the pull review. to a public URL. So you can actually see your product, your project running on the web and maybe who's reviewing it can access that URL

Speaker 1: instead of having to run the project in their machine. So that's a very nice obviously paid feature. And linters, as I've mentioned before, linters are very, very helpful for a number of things. If you access this talk and you click on these linters, that's actually a link for a talk by Flavio Juvenal. It's really great about linters. Some insights that we got from Vinta. Remember to warn people on Slack when things have changed, the status on the pull request review thing. So if you finished fixing the suggestions or you finished reviewing, alert the person on Slack instead of just relying on email or repository notifications because some people will sometimes check emails like twice a day

Speaker 1: And it might take a few hours until the next check. And also remember to always test first the feature and then review the code. Because if you test the feature and you find out the feature is not working It's not worth reviewing the code because it's probably gonna change. Something is broken and the author is gonna have to rewrite the code. So you're gonna have to work twice if you actually review the code first. And finally some real world examples. They are mostly from the Django repository. You can you I'm not sure if you can read it well from there, but It's basically someone send a pull request to the Django repository. The person assigned to review it said there's some conflicts. Please can you resolve this first? So it's been as polite as possible. The author is not very used to the pull request

Speaker 1: flow, he doesn't know exactly how it works, and then the author uh like very patiently explains something that for maybe for some of us who are already experienced with pull requests would be like super obvious. He points out what uh where he can find the the conflicts in his code like the the the error signs the equal signs that divide what's what's uh the current one what's the incoming change So yeah, I've highlighted here. He says, can you please resolve the conflicts first? And then he says at some point you must have pulled the rebase with master. So check for the signs, that's where the conflicts are Another example here is when I was talking about teaching, not just telling, the reviewer is again saying, oh, are you still struggling with that?

Speaker 1: Maybe if it was in Python 2 the error could be this, but if since it's Python 3, it's probably something else. So he's not just saying, oh, do it like this because it's the correct thing. He's actually telling how it should be done and uh why it should work like that and a suggestion below. Here's just someone suggesting for the person to look at the guidelines of the project. So having guidelines is also important for people to actually know how to contribute to your project. And here's I think the last example the reviewer suggested for someone to create a unit test for the changes they're making. The person didn't actually uh know how to do it he kind of explained to it to to the person where should where should the test leave because the person doesn't have the obligation of to of knowing where how how is

Speaker 1: the project configured And also there's a second part of it that he comments uh the the reviewer comments in a specific part of the code and instead of just saying this should be uh name it he says do you mean name and say uh if name in settings dict uh so he gave the the the author the opportunity of explaining him or herself And the author actually came with an explanation saying why he didn't think it should be like that. So maybe that's something you hadn't thought about before. And there's the opportunity for the author to explain And finally, there's one more thing. We do at Vinta we like checklists very much And we've created a checklist for code review and management. It's not something that you review you use it like for every pull request, but

Speaker 1: something that you might want to go through for establishing your code review and management process. So if you access this URL bit. ly slash pull request checklist you can access it. The interesting thing is that it is cache-based so if you select the the the the options there there are checkboxes they stay selected for like next time you access it It's open source, so if anyone anyone wants to contribute, maybe add something, change something, find a typo or something like that, feel free, and it's free to use and share. And that's it. Thank you very much

Speaker 2: Thank you for that. That was very interesting. Um so I'm thinking of a scenario where you have like a senior developer who um is overseeing the work of several junior developers and you know the senior developer obviously reviews the code for the junior developers but how do you it do you have any advice for doing it the other way around when there's not necessarily as much oversight of the senior developers work And there might be like political conflict there, it might be a little bit weird.

Speaker 1: Uh just so just so I'm clear, your information about like the junior developer reviewing the senior developer's code, is that it?

Speaker 2: Possibly, yeah.

Speaker 1: I think a good idea for that would be to have multiple people reviewing the code, maybe somebody who's a bit more experienced, and also having the junior developer review it so he can get the experience. and uh actually learn the like practices that he may be not familiar with, but I wouldn't uh I wouldn't suggest to have just a junior developer reviewing it. I would always suggest to have something, someone more experienced reviewing it with him or her Does it re I don't don't know if it answered your question? Yeah. I'm sorry, uh I'm not sure. Yeah, okay.

Speaker 3: All the reviewers test the code, like you download it and test it. If for example you have uh to have two reviewers, the two reviewers download and test the code. Uh

Speaker 1: you mean like one person reviews the other person tests the code? Uh

Speaker 3: no, if uh uh all the reviewers that are assigned to that pull request should download and test the code. Because that that's really time consuming. So

Speaker 1: Uh I'm I'm not sure if I understood your question a hundred percent. So you're s you you're like are you asking about um if the the if a same person should review and test the code, is that it?

Speaker 3: Yes.

Speaker 1: Oh Well, I think that's uh that's my personal opinion of course. Uh but I think that's worth it. Like it it is time consuming, of course. But I think you gotta have uh an understanding of how the feature works in order to review the code properly and you won't do it if unless you actually test it manually or do the QA. So I think it's worth it

Speaker 4: Okay, that's all the time we have. We have uh another talk in a couple minutes here. So thank you