 So it's been a while since I've seen you. How have you been? Yeah, yeah pretty good a little bit of traveling but I can dial in now Yeah, it's been it's been a crazy little while I know it's been it's been a little bit of time since we've been able to make our video I know we've we haven't done very many together We were kind of hoping to do one every every week or every other week Chris and I get together and just at least chat once a week to just touch base But it's been a while and we thought we need to get something out so that you guys know We haven't completely just abandoned the idea of making these videos So today we thought we'd talk just briefly about what it takes to make a good PR and The elements that kind of go into the basics of a really good PR for the mycroft community And so I think today instead of doing our like intro and all the rest of that We're just going to jump right in and get right down into business and crank out a really quick video Yeah, yeah, so I think this is this is going to be some tips for how to make a good PR And also if you're helping us out by reviewing PRs what what to look for In there, you know mycroft is a very large Fairly large open-source community And we get a lot of we get a lot of contributions many more than we can review ourselves We're a pretty small team and we you know, we kind of have to stay focused on things like the getting the mark to out so Often, you know, we get we get a lot of contributions that are really cool features but not necessarily essential features so The ball that the community can help us review those as well the quicker we can get them in there and and get more features out for everyone so This is intended to help out on that front as well. And with that, let's get right to it Here we've got a PR from Fossland or okay It's got the titles obviously fix remote settings over at startup so you may not really Understand the contents of this but that's all we're really interested in We just want to use this as an example. So whether you're making a PR or reviewing it You you want to make sure that first and foremost the code that you're submitting is going into the right Branch it's going to the right place. So you'll see directly under the heading. We've got Fossland wants to merge one commit into mycroft AI colon dev So that's the place where the code is going to So it's going into the mycroft repository onto the dev branch from Fossland's repository from the feature remote startup on change branch, so One of the one of the things that we that causes a few problems is if people try and say push a Change directly to our master branch for example, or maybe it's going to some other feature branch It should go to dev by default Unless you change it, but if it is wrong then a whole bunch of the automated things Just not going to work because you know, it's it's looking at two different code bases and saying, okay You know can this cleanly merge into this other one? If not, then, you know, we'll throw some errors at you and stuff And also when it runs the tests on your code It's going to run it based on if this code was merged into this branch These are the tests that we'd run and and this is the outcome of those tests. So So that's it's a very critical one. Um, so anytime you're reviewing a PR Just just have a quick glance at that and make sure that's right before you spend any time anywhere else Cool You'll see on the right, you know, we've got a few labels Some of those are automatically applied some of them some of them we add ourselves You know, you'll kind of learn about that as you go But most of the time we'll handle that stuff for you And then I think the second thing that you really want to look at is is the checks So before we bother reviewing code A lot of the time it's good to see whether the tests are passing or not and whether You know There's a few different checks that happen So if we had all the way to the bottom of the screen, uh, we will see that there's some some red and some green So the the main red there is that the code hasn't been reviewed yet So that's expected because we haven't reviewed it But the the thing that we do want to check is that all of the Checks have passed. So in this repository there are three separate Checks and if you click show all checks on the right Um We'll we'll see that, you know, there's a few different things that are going going on there Um, so we're going to go into detail about what they are because they're going to differ from For different projects different repositories, but um, you know, it's most likely That the the code builds correctly That any unit or integration tests are running and passing properly Um, you know, there could be some other things in there Uh, so that's that's what they're about. So if they're not passing then That's then your call to make about Is this is this PR that you want to review right now? Um, so if they're not passing It's probably something that the author needs to go and and have a look at and fix up If they're struggling and they Don't understand why it's failing Then that's when we can go in and have a look and and try and provide a bit of guidance to them But but generally if the tests aren't passing it's the checks aren't passing That's um, that's probably something that the author the code author needs to to have a look at Um And so this saves us a lot of time, you know, we don't want to bother reviewing a whole bunch of code that are being told by the system doesn't work. So So, you know, we want to save our time. We want you guys to save your time as well. So Cool does that make sense so far. Yep Sweet um, what are we moving on to so then Um, once we've we've made sure that those basic things are in place We'll we'll start to look at the details of the PR itself. Um, so mycroft has, uh A A template that people need to fill out when they're creating a PR That can differ from from different repositories, but then we have a sort of global default as well uh, but primarily it it has to include and A description that does a brief overview of of what the change is and why it's being made um We include how to test it. So because you wrote the code, um, or because whoever wrote the code should know how to verify that it works or not Preferably that's going to be in the form of an integration test or a unit test or something of that nature, but it might also include, you know Say this thing to my, you know run mycroft and say this thing and it should do something different or in this case you know Ensure that the skill updates actually work properly. Um edit edit something locally because this is this is about not Um forcing updates from from remote onto a local system You know edit edit the settings locally and make sure that that they don't get overwritten So just trying to provide nice clear instructions for a reviewer on on how to make sure that This change is really really working because it probably seems very obvious when you're When you're the one making the pr because you you understand exactly what the what the problem was What your solution is? Um, but that's not always as clear for people when they're reviewing. So We try and encourage people to do that as much as possible Well, it also makes your job easier, right? Like if you don't have to spend time Kind of deciphering what it is you have to do like even if the description is bang on You may not be immediately obvious What it is that that you need to do to make sure that this works properly So if you can get almost kind of a cut and paste Instruction it makes it way easier to take a half hour to review a pull request That has good tests and you can push it down the pipe as opposed to something like you look it up And you think well it might even take me an hour and a half or two hours just to Just to go through to figure out what it is that I need to do to make sure this works properly You're less likely to take that That addition on to to your day. And so that pull request will probably sit Yeah, exactly. Um Yeah, like if you're making a pull request making it as easy as possible for reviewers to to do their job means that they're going to be more likely to review your pr And you know as you said like I'm sure that Given a little bit of time you can figure out. Okay, they made this change So I should do this thing to make sure that that's still working and and as a reviewer You do need to try and think about some edge cases and things like that But but yeah, if you can if you can provide some really straightforward You know run this command instructions then It makes it a lot easier and you'll get a reputation for having easy to review commits. Yeah Which you know means that people are more likely to You know just grab one while they're you know between jobs or you know, if they've got an hour in the evening They might you know, they might look at a few pr's and say well, I'm pretty sure that that you know that person does good Good descriptions and good test instructions So I'm sure that I'll have time to sort of have a poke at that one But I'm less sure about these other ones Yeah, and case in point when we were trying to decide on a good pull request to do this video for We looked at the the reviewers and and the authors and said oh well forzeland almost always does Really good pr's and so we actually kind of out of the hundreds that we could have chosen from We kind of narrowed it down to which ones of his are kind of active right now Because we knew that because we knew that he follows the process through and makes it easy for the reviewers Yeah, yeah absolutely So yeah, and if if someone hasn't got the Detailed information in there and you're you're Looking to review it then feel free to push back on them and say how do I test this? You know, what what is your recommendation for me testing this or your description? I'm not quite clear from your description what the problem that you're trying to solve is or What your intended solution is or why this is really a problem or you know, whatever like Definitely ask questions. Um, and if it's your pr and people are asking questions then It's because they they want to get your change merged in so, you know Try and be helpful and You know, no one's asking questions just for the sake of Commenting, you know, there's there's no gamification. There's no point system for how many comments you can make or anything like that It's just we're all trying to help each other out. So um, so the more that we can help each other do that Uh, the quicker we can get things merged and the better better microp becomes for everyone. So um Do you want to talk a little bit about Good descriptions Yeah, so for a good description Oftentimes it it needs to be right to the point and you have to strike that balance between providing a wall of text and not providing Enough description. Uh, so generally speaking you try to do What what force lund has done here where he provides some context as to what's happening like he's talks about What the current functionality is actually doing? And then why this pr is needed? And what it does to kind of address that issue And and that's really important because when you go through if if you Give a high-level overview of what the pr is attempting to do to rectify the the situation When you review the commits, you can actually view it through The lens of what the person was trying to achieve, right? If you just simply review What they did without what they were trying to achieve You might not get let's say The best code for the job And I I see this a lot in When I deal with clients for work The client will come to me and say I want a solution that does X but they won't actually tell you what they're trying to solve. And so if you just blindly go down the path of Trying to um Work off of some specific thing you'll find that The solution you come up with is not the solution. They ultimately wanted It's just what they thought they wanted and it's the same thing in this case if you don't have a good description Uh, the solution that comes in May look like it fits But it might not actually be what you're trying to do Or at least it may not be the optimal route for the approach that you're trying to take And so a good description helps everybody it helps you kind of Cement the thoughts that you that you have and maybe even and I've had this when I write a description I go back and make an additional commit before I submit it because I realized that um, I I was missing something or Something clicked and I had a better way and so a good description helps you it helps the pull request and it helps form an idea of what you're trying to achieve Yeah, yeah Yeah, and if you're not if you don't have that why Often you'll get the question of like well, why didn't you just do blah because You know the person is thinking. Oh, well if you're only trying to do this little thing Which is what they you know based on your pr title or something like that um You know, it won't be clear as to why you needed to make all these other changes and and all that sort of thing so um Yeah, again, it just helps helps everyone get on the same page. Um and helps the review process go much quicker um The other thing is, you know like commits this becomes part of the history of the project And so when people are looking back and they say they look they go, you know, why Why is this code the way that it is? They can go back and look at the um the pull request and and see all that detail And that might be in a week. It might be in five years time like who knows Um, cool So then the next the final bit uh that we want to look at um is the commit history I think I think I find it good to just have a brief look at the commit history This one is obviously really simple. Uh, it's all in it's it's a fairly straightforward change. So it's all in one commit um Uh, and you can see that We've got a really nice commit message there that has some some good detail um So so I'd feel pretty confident about reviewing this code um, when you when you get in there and and there's you know, 27 commits and and they're all uh edit edit.py You know, yeah typo fix typo fix typo fix typo fix. It's like, oh man, this one this one might be a bit painful. So, uh But that's not a reason not to reveal it It just might it just might be something that you provide a bit of feedback on to the um to the code author as well Um, and we can't we do have the ability to to squash commits when we're merging them, but we'll we'll get into that later um Yeah, I think I think that's about it. Like, you know from the from when you're first jumping in to have a look at a pr the things that you want to The things that you want to really take note of first before you You know dedicate some time to actually reviewing the code Which I think will save us all a lot of time and and mean that we we get the best commits merged more quickly So in in this video this this was kind of a a short quick and easy What makes a good pr and and really it boils down to Make sure that you have a good description about What you're doing Why you're doing it and possibly how you're doing it and that leads into making sure that you have a good How do I test section? Uh, we also talked a little bit about making sure that the automated tests is are passing If there are actual automated tests in the branch Um, and aside from that Try to make sure that your commit messages are good that you don't have a lot of duplicates where You know, I think the example was used fix a typo. Like if you just have seven commits saying fix a typo Um, maybe you should take some time to clean that up before you submit your pr and that that's basically it Yeah, so next time, uh, I think we'll we'll start getting into Uh We'll start getting into what you should look for when you're actually reviewing the code itself So today we we just looked at the pr and we'll we'll get into the code Um Provide some of the tips that you know things that we look out for Uh, as well as how to actually get the code onto your machine so that you can run it locally and But that's about it for this one. So Until next time. Yeah, hopefully we don't have as long of a lag although Christmas is coming up. So We might have another Longish break between our next video. So hopefully we don't have we don't make a a long pattern of this Yeah, yeah, totally. Although with covid. I'm I'm just gonna be At my desk through Christmas. So who knows? We'll see if we can squeak one or two in then Yeah, uh, but yeah, if you if you like what we're talking about here then then make sure to Hit the like button and it feels weird to say this sort of stuff But hit the like button or like put a comment about you know Think your own tips for reviewing prs Or any questions that you have that you've run into And if you want to if you want to help reviewing prs at microsoft then uh, then Also you know Do a comment or just jump into our github and uh, and there's just plenty to choose from Come join us on microsoft chat. There's plenty of friendly people that can point you in the right direction So yeah until next time Until next time. Ciao