 All right, so It's not been too long since we've kicked out the last video although it took us a little while to to make it over the holidays, so I Guess I'll start with what I've been up to largely. I've been going kind of crazy with My 3d printer. I discovered how to use CAD programs. And so I've been printing like shelves and Small things for knick-knacks and stuff like that with with customized pockets like my wife has like these essential oils She's really into the essential oils And so I made a shelf that has like the diameter of the different types of Bottles that she has so that they don't just kind of like can't just get shoved off the shelf And then I have I have a bunch of these tuxes that I've kind of collected over the years I've had like seven of them and they are all just kind of hanging around on my desk And so I ended up making a vertical shelf That's sitting right to the left of my monitor here with all the tuxes stacked on top of each other. So Yeah, discovering a CAD program was either really good or really bad for me. I guess it depends on your point of view Yeah, cool. I am I Have been really muting myself and from the outset I want to apologize for the some of the audio in this in this video Someone is doing some kind of renovations in our building. So there's there's Drilling and hammering and concrete cutting going on So we'll try and chop out as much as we can but you might you might hear a bit of that The joys of apartments, hey Yeah, yeah, but yeah Really excited that we yeah, we put the last video out Last week and it seems like we're gonna get into a good cadence in 2021. So I'm really excited for for what we can come up with and Yeah today, I think we're we're gonna look at So the last video was looking at you know PRs and particularly, you know, if I'm submitting a PR if I'm starting to review a PR What are the things that I that I should be looking at? You know before actually reviewing the code or anything like that. And so we wanted today to get into When we start to look at the code How do we do that and how do we use the GitHub interface? You know, what does that give us? rather than looking at it on our local machine and then starting to look through a fairly simple PR and You know take a look at the changes that are in there start and have a bit of a talk about have a Look at the changes in there and have a bit of a talk about, you know, what are the things that we notice and what are the things that? We might look at We might make comments about Anyway, so I'm hoping that the drilling Holds off a little bit, but I think we should just die straight into it. Yep. Let's get right into it So I think let's look at Homesystem skill because there's a few PRs that have been sitting in here that we need to take a look at and Let's let's go the second one the feature entity availability check So We've already looked in the last video, you know the details of this PR so So we want to just jump straight to What is the code changes that have actually happened and and what are we looking at there? So? In the tabs at the top will jump to files changed and that's going to give us The you know all of the changes that have happened and so because this isn't Too big of a Of a PR we can just review all of them at once Like we can see that that it's broken into two commits, but you know that we're only talking about 31 new lines and eight deletions Which you can see in the in the top right corner So so I just I just review this all at once if it was a bigger PR, you know, often you get PRs where there's You know 20 fire get 20 different files that have been edited, you know or one file has been has had hundreds of lines changed and you know, it's addressing different things with different commits and And in that case, you know, I'd go through commit by commit and But but in this case we want to give it simple if we maybe it's worth saying actually if we if we did want to go through commit by commit In the top left of your screen, you can see changes from all commits and if you click on that all commits You can actually select individual ones, so if if it was You know one PR that contained a number of substantial changes And hopefully that the person submitting the PR had had structured their changes, you know, really nice way So if if it was if it was a big PR And you know, there are a number of significant changes, but the you know The code author had been really good and structured their commits, you know in a logical way where you know each each Each change that sort of came together with was its own commit then then we can go in and we can review the changes on a per commit basis and Sometimes you you really need to do that Because it's just not possible to look at hundreds of lines of changes and know, you know, how different things are going to affect each other So so that's a really useful feature, but for this one, we're just going to do it all in one So you can see that in this it gives you a really nice way of seeing what's been added What's been deleted and what's been changed? So sometimes they're innocuous, you know line 71 Looks like a tap disappeared Yeah, yeah, which is which could be a winter, you know, a lot of the linters will do that for you, which is good And then we can see that there's a new routine for the entity availability check and And then they've Added a call to that to that check From a number of places throughout code And So What this interface does is a it lets us see the code before we load it onto our machines, you know, we don't know We haven't had any malicious people in the community trying to like get get malicious code down onto machines but you know, if you're if you're Downloading code from other people that's gonna run Particularly, you know, if you're not running in a VM or have other security measures in place You know, you want to know what the code is before you just pull it down on your computer and run it and say, yeah Do whatever you want So this is a really smart way of you know, just doing that basic check first up And the other thing this interface lets us do is it lets us comment on really specific pieces So, you know, instead of just writing having to write a comment in the in the conversation view of the PR That says, oh in that section, you know around line 120 ish there, you know, I thought that the the variable name could have been Could have been better or something like that, you know In this if we had something like that here, we can click on one of those One of those pluses to the left Or not so after I Figured out that I should be logged in because why wouldn't you to be able to make a comment That that changes the orientation of your screen and this is what it actually would look like if you were logged in so now that you're logged in so I'm just gonna You can see along the side here that now when I hover over a line There's a little plus and this was what Chris was talking about earlier or was talking about if I didn't cut it out We'll find out in the future Exactly and this is this gives you the ability to comment on specific lines And I believe you can actually do a like a multi-line comment Yeah, if you click and drag on the plus you can do a multi-line comment highlight a Section instead of going line by line Yeah, that's a new one, but it's so good Yeah, so it means that you know you can you can make a comment about very specific piece of the code And then it's really easy for everyone to see exactly what you're referring to You know rather than having to just try and describe where in the code something happens So so yeah, that's that's a benefit here So we check the code before we load it on to our computer gives us a really easy way to provide comments on the code and when we're Finished with it it provides a really easy way a really unambiguous way of saying what we Think of the code So, you know once we've gone through and checked it all And you know done any comments that we want to do at the very end we're going to click that green button in the top right corner that says review changes and That gives us the ability to do one Final overarching comment that will come above everything else And it lets you say are you just giving feedback without an explicit approval or you know or anything Are you approving and in this case, you know from my cross perspective if you say that you're in your view This code should be approved. That means you yourself are putting your reputation Staking your reputation on the fact that this code could get immediately merged into the code base and pushed out to 50,000 devices and get used immediately You know that that to us is what approved means it doesn't mean, you know, yes I think this is a good feature doesn't mean Yeah, this sounds like a good idea. It means the exact code that that this PR Has at the moment could get merged right now with no further view no further review from anyone else and and You know and I would stake my personal reputation on that being the case um So no pressure No, no pressure. It's like, you know This is this is what we really want to want to get to um is that we can Um You know, we can share the load of reviewing PRs around but but in order to do that, you know, we need to have very clear um responses of whether Whether I believe that the code can be can be added or not because you know, it's really easy to say Yeah, it sounds good. And then You know another maintainer comes along and looks at it and goes. Oh, yeah, I think I think You know Jeffrey here has has has looked at it. He seems to think it's good. So I'll merge it in But of course Jeffrey may not have actually read all the code and then we don't know, you know, what's in there We don't know anything so That it's it's really We want to try and get to this point where we're either saying it's it's totally okay to just do a comment if you you know if you've reviewed it and You had some feedback you wanted to provide but Either you think some changes need to happen or you're just not confident that it could be merged yet And that's fine. Just do it as a comment or as a request changes um But if you if you really believe that it can be can be merged in then and we want to prove And and that's the one that's going to help us to get uh changes Merged into the code base and shipped out to everyone the quickest so Um, you know, there's some pressure, but there's there's also some time off there. So Yeah, and I think it's really important too. Um I've found that in the past outside of the computer world if you set expectations for people to live up to and they Care about the thing that they're doing oftentimes more often than not the they're going to live up to That expectation so if we set the expectation that You know if you say that this is approved it reflects on you then people are more likely to You know take that more seriously and and they'll live up to that Yeah, it actually it reminds me of um, so at university we had a mountaineering mountaineering club Sorry, I need to pronounce my t's. I'm australian. Sorry about that But uh, we had a mountaineering club. So an outdoors club and um, you know do Climbing and and all sorts of different stuff Anyway, I had one of the trips a caving trip. There was an accident there someone Didn't rig up their abseil device correctly and It got checked by Two or three people and then they went to abseil and and fell And thankfully, you know, they were okay. They they did I think in that case they broke a bone and it was you know it was a Needed a rescue From from the team that were there. Uh, so, you know, it wasn't a good scenario But talking to the university afterwards, you know, anytime there was even a small accident We'd always do a a big, you know post incident review And um, you know talking to the university they were they were like, you know, why wasn't someone checking it? We're like, well, we had We had, you know, two or three people checking it But like all of them kind of looked at it and assumed that and thought that it was fine Um, and so the university said well, we need to have more people checking it And and I pushed back and I said well actually I think the problem was that we had too many people checking it So because we have three people in a line Person number one assumes that someone else is going to catch it And so and person number two thinks well, someone's already checked it and someone else is going to check it They asked me so like, you know, sure it looks fine to me And then the third person goes well two people have already checked it like it looks fine and where You know, that's where we actually went wrong. And so we we changed things so that there was one person reviewing reviewing, uh, reviewing You know in that case the lay devices And so that put the responsibility solely squarely on that one person and We didn't have any change. We didn't have any problems after that um, or you know I'm sure that there will be problems in the future. But like, you know, you can you can then I don't want to say important blame, but there's just the responsibility there Um, and we know we know that when we're doing something that we are taking responsibility for that And so that's what we really want to get to here as well There's a reason why they call it get blame Yeah, just saying So Moving on. We've uh, we've taken a look at the the web browser. Obviously, we're not going to approve or comment anything right now um, I I have actually gone through this previously. Um, I specifically You know part of its laziness, uh, I've had I've had a bunch of stuff that I've had to do in my personal life But this this pr in particular has been on my list of things to actually check and when I looked at it I thought it might be a good example of of how to pull down the code for this type of video. So the uh This pull request Ultimately is is checking the state of an entity Before taking action on it and previous to this Uh, if an entity existed, there was no check to see what its current state was this if the In-home assistant you can have an entity Or an end point might be a light or a switch or a thermostat or whatever. Yeah, exactly You can have an endpoint that's defined That has some sort of different state like unavailable for example Um, where this will happen Particularly with light bulbs if someone's gone and flick the light switch on you then the power has been cut to the entity It's unavailable. That doesn't mean that there is no entity So uh home assistant Will register that yes, there's actually an entity involved in this And so the previous code was looking for you know, if not entity And so that's just saying well if the entity doesn't exist anymore Like I've deleted it from home assistant then the code would do whatever that's supposed to do This pull request is Supposed to prevent the home assistant skill From attempting to take action on something that actually can't handle the action right now So that's that's the whole purpose of of this pull request Some of the things that immediately stand out to me is uh, is that you know on the method itself They've they've used an underscore at the start of the method name And so if you're not if you're not familiar with this convention, it's a private method in python um So it's not it's not like other programming languages where You Cannot access the private method. It's more of a convention rather than an enforced rule But what it does is say, you know, this is an internal method only Things outside of the skill shouldn't be using this And so at any point in time in the future we can change the functionality of this method And only take into consideration You know the skill itself rather than you know anything else that might be trying to piggyback off the top of it so Moving to the next character We're gonna take this one character at the time. No kidding. Um, but uh, you know Naming naming variables and naming methods is always um is always a big part of of Both writing and reviewing code and and reading other people's code. Um Uh check availability Seems like a pretty good method name. Um I actually made a comment here about self.log.debug because I think that Um because we actually have a dialogue down here that that's going to tell us when something's available We don't need the debug to tell us the current state. So I just put a comment in here saying probably should remove the debug code Mm-hmm. Yeah, okay The check availability what would be very helpful in this case would actually be to have doc strings Uh a doc string generally is used to tell someone else About the method that you are you're doing so what type of variable you take in If you've taken in a variable as an argument What the output is if something returns If it if it has some sort of external dependencies some of these things are going to become less important as python is adding things like type hints and and other Other things but in the meantime, it would be really helpful if I knew what this What this method was attempting to return so we take a look and it says well, it's going to return true or it's going to return false um When we were looking at If not ha entity or not self.check availability It returns nothing and Sorry that function will return as in it quits the It quits the loop of that skill Right, so it's an early exit It would be helpful If I knew that because of the way that this is particularly worded It's not self-taught availability It would be helpful when I came up here to check this that I knew that this was going to return true If the device was available Right or if the device is not available because it's unclear from the method name What the what the intent behind this is yes, we can parse it out We absolutely can take a look like there's nothing wrong with the code In here, um, it's clear. It's in size. I understand what it's doing Uh, it would just doc string help much better Like if I'm if I'm jumping between right the point of this was we'd already I We took an aside and we were just looking scrolling through the code And I looked at this and then I had to scroll back up here to read this section again to figure out What is it actually returning if I had the doc string I could have just read the doc string like oh well It's returning, you know, it's returning true if the device is actually available Yeah, and it's about like how much you know any human no matter how intelligent you are No matter how experienced you are with code. There's only so much that there's only so much Ram that you have in your brain and you know When you when you have a particular Flow that you that you have in your in your head You know at the moment And then you go okay, and I need to figure out what that's returning So I'm going to go up to there and if it's a doc string You know, you just read what does this return it returns, you know True if if the entity is available. It's like great. That doesn't require any more brain space Yeah Um Well, sorry her random pop-up Whereas if you actually have to you know work through the logic of that method even even quite a simple method like this one It's it's taking up that active memory And probably bumping some other things and then you have to then go back and and rethink through Wherever it is you were coming from so Yep, I feel the obligatory xkcd. Have have you seen that one where The guy is is really concentrating like he's got this flow short working in his brain And then someone comes in and asks him if he wants a sandwich and then the next frame is poof everything's gone And he starts from the beginning Yeah, yeah, yeah Exactly So the other yeah doc strings definitely a great great thing um The other thing that could have helped this even without a doc string is actually changing the method name to a question For example, if the quite if if the string was is entity available Then that leads to like a natural understanding of a true or false like what the output of this Method is going to be So there's a bunch of you you see like this this is called a bunch of times the same kind of Statement of if not Go ahead Well, so what uh, one of the things that's due out to me and what I I think you were getting towards is um You know when we look at where this method is being used in each case We have a if not ha entity or not check availability and Uh, you know do something um, mostly early return Or maybe you always early return anyway, but it looks like in every single in every single case We always do this along with calling this method. Um, and I think that's a really good code smell for His code smeller I don't know I would have said uh, well you're going with dry, right? Do not repeat yourself. Yeah. Yeah. Yeah the old drive principle do not repeat yourself. Um Basically a code smell is like, you know You start to see patterns and like when you see different patterns, you're like, uh, I think, you know, this thing might be, you know Might be happening here. Um, and so in this case every single time we call check availability We're also calling. Well, we're also checking if not ha entity and I think that's generally a really um A really easy way to see that maybe We should check whether ha entity exists or not in The method itself Rather than doing it every single time Everything every single time in the conditional Yeah, I think that the the original author here was following the pattern that existed previously Right, so you're seeing You're seeing that pattern existing But like you said, um, if I'm seeing this exact same code all over the place I might even uh make a method itself Even if I didn't want to add this this check into because Uh, so the counterpoint to adding the ha entity check is then Your method is no longer doing a thing. It's now doing two different things. And how do you quantify that appropriately? So It might be better off in the long run to have a check availability And then call that in another method that that just says like, um something like Is actionable and is actionable checks the output of this here And so you run is actionable and it runs the code and it either returns or it doesn't um That's that's just another way to kind of tackle that but at any time that you see The same literally the same cut and paste code Uh, it is a good indication that there's probably a better way to do this Now there's nothing wrong with with this per se right because the original and it dot pi had these lines in there but Um, if you're going in and you you're taking the time to clean up the code anyways You, uh, it might be a good opportunity to rethink how this might be accomplished Um, so that pretty much is the totality of what we're looking at here You can see that I've actually come in here and viewed some of the stuff Because like I said, I've I looked at this before I've looked at this a few times I just haven't actually gotten around to testing this yet. So This little thing just tells us like don't show me these things as I've gone through and viewed them and um Yes, they're technically changes, but in these cases we're talking about dialogue changes and For two of these language. I actually speak them. Um, and the check I just have to assume that's correct. So Uh, you know, I marked them as viewed and and I'm just not going to bother making comment there Yeah, but that's another useful, um tool in in this interface You know, particularly when you're talking about larger changes, um, that are spanning across many files Uh, I definitely use this one to keep track of what I've checked and what I haven't Um, yeah, and you know, you get that beautiful progress bar at the top That helps to gamify your experience and encourage you to finish it off It didn't work in this case Tony if you're watching I'm sorry, uh, this has nothing to do with you No, but thank you for uh, for a good PR Um, and for the other all the other work that you've been doing on on particularly our own assistant It's been really cool Okay, so Uh, we've made we made a comment. Did we yep, I made a comment up here about just not needing the uh Not needing the debug code Yeah, yeah, very cool. Um Yeah, yep, uh All right, so assuming We are happy with this code. We know that there's no, you know It's not trying to install a rootkit on your device or something Um more likely, you know, it's not just going to sit in in some endless loop and you know, hollow your system resources or something silly like that Um It's pretty clear. It's pretty it's very logical. Um, you know, it addresses a real um A real issue. I think, you know, it's good on a lot of those fronts Um Yeah, we'd probably be better if we if we had a doc string in there Um, just uh, just to help people who are coming along to to really quickly understand, um What the What the methods purposes, um what the Input Um argument is what like what what do we expect ha entity to be like if I pass in A boolean value is that gonna You know if I pass in a Sorry about the tapping if I pass in a uh by passing a boolean value or or a you know A random object or a you know, whatever. Um, is is that going to handle those? Um Probably not Um Yeah, and then and then what what's the return? What's our expected return values and in this case it's a boolean um Yeah, but I think I think it's a simple enough method that uh It it doesn't really need any inline Um commenting other than a doc string Well, and in actual fact they've they've kind of Done like the first line of the doc string as a comment above the method. Um, so partly it's just shifting that Um, maybe maybe tweaking it a bit but shifting it down, uh You know inside three pair of three quotes and So I just want to hone in on that just a little bit The the difference between a doc string and leaving comments in your in your method Comes down to your id right or If you're using repl so you're you're interactively using python and you've you've imported somebody's method You can actually use the built-in python tools to get at the doc strings to Um, you know either your linter will do it or whatever for your ide So there are some actual technical difference between having a comment and having a doc string um Yeah, absolutely, uh and in in a lot of our projects we use we automatically extract the doc strings to Um populate our technical documentation. So if you're on our read the docs site for for you know, if you're doing skill development And all of that all of that documentation is automatically pulled out of the code So that means that you know, we don't have to maintain separate documentation to our code. Yeah, it sort of self generates itself So I had a lot of fun today I'm kind of digging around through through some code that I've looked at significant amount because hashtag lazy I'm sorry, tony. I've looked at your code a ton. I just haven't gotten to the point to To actually run it on my own and then when we started doing these video series I selfishly wanted to hold this one because I'd already looked at it several times and I thought it would be a good example for us to run through. So We will get this through But it'll happen when we finish off the the video series for This uh this set. So we really covered today Using the git ui. So specifically github. We used a lot of the features in github for Doing a review. We added some comments, you know, we took a look at the the file in the web browser with the express idea that um, we believe it's a bad idea to just Pull in somebody's uh Code and then run it without taking a look at it So the the web ui kind of gives you a pretty quick glance at what's in there and you can Quickly scoot through and make sure that there's nothing malicious or even accidentally harmful in in the code and so ultimately that's that's What we wanted to impart with this one is just to show you some of the stuff that you can do as a reviewer with the web ui and Uh, I think next time we'll get into Actually pulling the code down onto your system and actually showing you how to run a specific pr Cool. All right, uh, yeah, so that that's it for this video. Uh, I think um If you have any questions about whatever we covered then please feel free to leave a comment like it Ask us questions about things that you want want to know about for future videos And yeah, come and join us in the in microsoft chat in the in the dev channel or uh, or Take a look at you know, some of the git ribos in in microsoft's um github There's uh, there's a few prs there that you know be happy for you to take a take a look at and review Yeah, comments would be really great because for the most part we see the the views on a on a video But we actually have no idea whether we're we're even connecting with anybody out there So right now, uh, we're just kind of pissing into the wind as they say They do they say that in australia for sure. It's just who knows what it means Yeah, please leave a comment, uh, you know, we want to make sure that Your frustration level is just precious Oh, we just leave it there. Please leave a comment. Uh, yeah, we want to make sure that that we're spending our time efficiently um, and so You know knowing that people Value these and and get value out of these Is really important to us. So um, let us know Until next time then Until next time