 So our next speaker give a big round of applause for Dougal, Matthews and Effective Code Review. Thank you. Okay, so I've been, well first of all thanks all for coming and for having me here. I've been thinking about Code Review for quite some time now so I'm quite excited to sort of talk about it and just sort of start discussing about it. That's kind of my main goal from today is that people are talking about Code Review more than I've had some success. So first of all just to give you some context of where I come from. I mean what I do isn't actually too important for this talk but it's useful to know where I'm approaching Code Review from. I work for Red Hat and then I'm working on the OpenStack project which is the bottom logo. And then I work on triple O inside OpenStack. And the key thing about all of these is everything I do is open source and public so all the Code Reviews I do are also public and open. So this means that some of the things I'm going to say are specific to open source I guess and maybe don't apply to a closed environment but I think still a lot of the things do apply. And I also run the Python Glasgow user group in Scotland which is completely unrelated but I just like to show up a cool logo whenever I can. That's an inside joke which I can explain to anyone later if they don't understand it. I'm also doing a bit of an experiment in this talk. I've scheduled some tweets and that's my Twitter account. The O and Google is a zero. They'll be tweeting out some relevant links to the talk. If there isn't two out by now then it's not working. So yeah, we'll see afterwards. But if you're looking for references to any of the papers or blog posts I reference they should be shared on my Twitter. First of all I just like to try and engage the audience a little bit and see where people are at with Code Review what you're doing. And to get a baseline how many of you will raise your hand if I ask you to raise your hand? Okay, that's pretty good. I guess we're talking about 90% so anything can be adjusted to that from now on. And who's doing regular Code Reviews at work? And by this I mean as either the author or the reviewer of a change. Okay, that's a good percentage. We're looking at about 75% there maybe. And who is doing them outside of work? So maybe open source or in like a side project, something like that. Again as author or reviewer. Okay, so that's a lot less than we're looking at about maybe 15-20%. Finally who actually does Code Review that looks forward to doing it? Okay, so we pretty much had everyone doing some kind of Code Review but then the number of people actually, so this is for the benefit of the video I guess and nothing else, the people actually looking forward to doing Code Review is probably down to about 20%. And this is the one that is the most interesting to me. I find Code Review incredibly valuable. There's so much you can get out of it and it's a really useful process but yeah a lot of people don't really enjoy it. So I'd like to start the discussion and see how we can improve that, how we can make it a better experience for everyone. I also done a straw poll on Twitter not too long ago and I found out that most people are doing Code Review, sorry, out of all the people that responded, everyone's doing Code Review up to 25% or up to 50% of their day. I think that's part of their development time. But I mean that's a huge chunk of time. So if we can improve Code Review we can improve what we build or perhaps more importantly we can actually improve how people enjoy their jobs. For anyone that's not doing Code Review, this talk is really more aimed towards people that are but I just got to quickly do a couple of quotes, things which if you need to persuade someone why you should be doing Code Review will hopefully be useful and there'll be some references to papers and blogs. So there's just three very quick slides. But as far as I'm concerned the only real reason to not be doing Code Review is if you're a sole developer on a project. And you'll actually find that, sorry there's a trick that I've used before if you're doing an open source project say on GitHub rather than committing directly to master, if you open a pull request you might find occasionally some people wander by and do a review before you merge your code especially if you've got watchers on your project because they get notifications when you do a pull request on your own repository. So first this is a quote from a book called Code Complete by Steve McConnell and he says that the average detection rate for a unit test is 25%, 35% for function tests, 45% for integration tests and in contrast the average effectiveness of design and code inspections is 55 and 60%. So in this he's talking, when he's talking about defects it's kind of bugs or problems with the code and he's basically explaining that Code Review is the most effective out of all these approaches to improve your software. I'm not sure I completely buy these numbers but even if it was on par with your testing that's quite an impressive amount of benefit that you can get. Next there's a quote by Jeff Atwood so he wrote this in a blog post and basically said the only hurdle to doing Code Review is finding a developer that you trust to actually do the Code Review and then once you've started he says I think you'll quickly find that every minute you spend in a Code Review is paid back tenfold. And this one really tackles the assumption that a lot of people say they just don't have time to do Code Review but really I think there'll be some upfront investment of course but once you actually start getting into the process of it you will find that you will reap the benefits over time. And then finally from a paper measuring defect potentials and defect removal efficiency academic names are great they found that formal design and code inspection is often top 85% defect removal and average it around 65%. So again this is like a huge number and this is quite good because this is from an academic paper and they share some of the data they use so there's actually quite a lot of method behind these numbers the other two are more anecdotal as far as I could tell. So with that aside hopefully you're all convinced now to do Code Review or as most of you are doing it anyway we can move on. But if anyone is actually still doubting the process for some reason I can bombage you with references and links so just come and ask me afterwards. One thing to... So one of the things about Code Review that's quite interesting is trying to compare the expectations people have with the outcome of the actual Code Review. And I think this is reinforced slightly by these quotes you'll notice two of them were talking about finding defects and bugs and I think that the reason they're talking about that is it's much easier to measure bugs than other things but there's actually a lot of other subtle side effects and outcomes from Code Review rather than just thinking of it as a bug hunt. In the paper Expectations, Outcomes and Challenges of Modern Code Review they said that while finding defects remain the main motivation for reviews reviews are far less about defects than expected and provide additional benefits such as knowledge transfer increased team awareness and creation of alternative solutions to problems. So what they actually done in this this research was done in partnership with Microsoft Research it might have been just Microsoft Research I'm not sure but they took some teams at Microsoft they quizzed the developers beforehand sorry, questionnaire rather than the quiz so they were asking them why they're doing Code Review what they're expecting to get out of it and the top result was finding defects and bugs they most of them said that's why we are doing Code Review but actually when you look at the sorry, and then after the Code Review has happened the research team then manually classified hundreds of reviews to manually find out what they thought was the outcome of the review and this is the results that they had so they found the defects was only 14% of the time so that only came in fourth in terms of the actual outcome of reviews so it's actually a lot more lower down than you would have expected I only called out two of the numbers because the paper had a graph in it and it was quite hard to see the numbers but they explicitly mentioned those two numbers so that's why I only have them but the order is at least correct so yeah, you can see above defects you're getting code improvements so the difference between defects and code improvements is the codes that made for review was perfectly valid and worked but it maybe wasn't the optimal solution depending on your context, maybe it wasn't performing enough or maybe there was an API they could have done it in a more elegant way I'm not sure with tons of options then understanding was a shared understanding in your team about what is going on with the code base so if you're not actually reviewing changes the only way you really find out what's going on sorry, is by looking at your git history which I don't think is particularly fun or you're just trying to use something and you're like oh this function's changed so yeah, it's just about making sure there's a shared understanding and everyone really knows how the project is developing and evolving over time and then the social communications which is obviously a very important thing in any team or community was also rated higher than defects so with these sort of goals in mind about if we're thinking about rather than thinking about looking for a bug hunt and code review, we've got to think about how we can maximize all these other benefits as well obviously finding defects is still an important part of it but we shouldn't really focus on that because it doesn't seem to be the main actual outcome but in code review we definitely have two distinct roles you have the author and the reviewer or you can have multiple at either side of the review process and if I go back to why a lot of people seem to have a negative view of code review I think it's often because the actual code review process comes along as almost an adversarial like you're working against each other rather than together you want to get your code merged but this person is just getting in your way by giving you feedback and it's kind of a tricky problem to solve this I'm not really sure why it happens I think it's definitely more prevalent in some situations than others and people that tend to be more experienced with code review are quite open to the feedback and it works a lot better but I think one of the reasons we maybe see this is I don't like the name I don't like code review as a name I don't really think we'll manage to change it because that ship has sailed but at least in my head I like to think of it more as a code discussion or a code collaboration thinking more about how we can work together as a team and it becomes an iterative process that you're both involved in so it's just really about collaboration and coming up with a better solution together you both essentially have the same goals in the long term although it can sometimes seem a bit of a conflict at the time when you're trying to figure out what the best solution is to reach that goal so what I'll do now is I'm just going to take a look at authoring changes and then we'll go to looking at reviewing changes after and just sort of a few bits of unsolicited advice I suppose from either side and you can pick and choose which you think is useful this first one I think is really it's really important and perhaps overlooked because you might think that this comes before the code review process which it does really but it's always important especially in open source projects to talk about what you want to do first if you don't have an issue or something describing what your goals are and you've not discussed it with the maintainers and you don't really know if it's even applicable or relevant to this project obviously this is something that you can judge yourself if it's a typo that's a no-brainer you should just send over a pull request or sometimes you might find that you can express what you want to describe in code better but you just have to be prepared that your pull request or your code review might be rejected just because you haven't actually made sure everyone's on the same page and I think it's also important in the work environment you expect to have a requirement description or something before you start working on the code so it's just making sure you know what you're doing before you write it it's always good to adhere to the project guidelines this is pretty obvious but a lot of people don't actually tend to look for them this is also perhaps a problem with the projects where a lot of them either don't have them or they make them too hard to find but you can always take a look at see how the project is working in general or commits that are being merged just try and fit in with how they're working and it makes the process a lot better so if they've got a good... they're always adding tests and documentation with everything and they're testing on different platforms then it's always good to make sure you're doing the same and fitting in with their development style providing context is really important with your commit or with your change, sorry, for review I find that review in code can be really quite difficult often you'll receive a diff for a project but you may be a lot looked for for a few months so you need to bring back the mental context of what the project was doing before and then figure out from this diff what they are trying to do now and just try to get that all together in your head it can be really quite tricky so you can get around this by writing good commit messages, good pull request descriptions just why are you making the change what do they want, what does it do just try and be as descriptive as you can and help them out and if you're missing the context from this image what do you mean peg? I don't know this is sort of related to the previous slide in a way, keeping your changes small and contained, this is about limiting the context required so you don't want somebody to have to understand the full breadth of a project and necessarily review something you want to be doing a small change keeping your change focused to what the actual topic is some people like to do like a two line fix in the file then they'll send you over a thousand line diff they've went around moving things around thinking they're tidying them up and that may be perfectly acceptable but it's better to submit these in two different review requests and this tweet I seen a while ago when I was preparing this talk and I noted it down and it's basically saying code review ten lines of code, nine issues five hundred lines of code, looks fine and for this it's basically some developers might look at this it sounds like an easy way to get my code words but that's definitely another right way to think about it and I think the reason you get more feedback on a smaller change is basically it's much easier for the reviewer to get that change into their head and just really get their brain wrapped around it whereas if you're trying to understand a much larger change you end up missing things because you're focused so much on the overall impact of it that lots of subtleties will be missed otherwise and this has always been kind of an instant and for me the easier smaller patches were easier or more efficient to review but there's actually quite an interesting paper this one is an investigating code review quality do people and past dissipation matter they do and they found that larger patches lead to a higher likelihood of reviewers missing some bugs so it's great, as I said I think this is a fairly self-explanatory and obviously one but it's good to see this is backed up with data I'm looking into the public Mozilla bug tracker and they merged it together with the bugzilla I can't remember what Mozilla used for bugs but with their bug tracker and their reviews and they combined it to find the trends of the bigger patches where they're introducing more bugs and so on smaller discs are good, touching less files are good when you actually submit your review I think it's good to think about that as being the start of the conversation you'll find that some people will submit a review and they'll wash their hands of it and they'll say that they're done so they'll send it over and they say, okay can you merge this please merge this, I'm done and that just really shuts down the conversation it's much better to send over the code and say hey can you have a look at this and tell me what you think you're basically inviting feedback and making that feedback loop much clearer I've had plenty of pull requests and things where people do not want the feedback at all and it just breaks the whole process and eventually you just find it stalls and things don't really progress from there one handy trick I find when you're opening this pull request I'm going to keep saying pull request but I just mean code review in general I'm not specific to GitHub at all when you open this request for a review I quite often like to go and do a quick review myself so I'll go and scan it on GitHub or on Garrett and I often find mistakes in my own patch at that point and I'm not sure why I think maybe it's the change of context from an editor to the code review tool that puts me in the right mindset so that's just a small tip which I recommend doing so when you actually your changes up you really need to try and relinquish ownership so this tweet was a response to the twitter poll that I ran about how much time do you spend in code review and to me this is just a sign of a really toxic culture I'm not sure if it's the fault the fault of the reviewers or the authors in this change or in this case sorry or both it's kind of hard to tell without more context but as the author of a change it's really important to just try and think analytically and subjectively about your change when you put it up and I think this can be quite difficult for people sometimes you feel quite protective of it you're quite proud of it you get kind of tied up with the EMI code works and then when somebody then starts to give you feedback the way I like to think about it is if you've looked at your code from six months ago you probably find it horrific so just try and think about the code you're writing today like you you will in six months and so really I think reviewing code is pretty difficult so the whole thing is about trying to make it easier for the reviewers to review your code and give you feedback just try and make the whole process go better help them to help you and then when we look at it from the front of you they're kind of like the gatekeepers for this project at least for code coming in so they have responsibility to make sure that the code coming in is good and suitable for the project and this is just like the authors have the same responsibility to try and make the best contributions they can for the project and the key thing really is that you want to make sure you're sharing the responsibility I personally see the reviewer of a code or reviewers as being responsible for the change as the author is and I think this is where some tools like the blame are a bit broken because that will quite often just get you the author of the change depending on how the code is actually merged and submitted eventually whereas it would be interesting if we could have a better way of integrating with code review tools and you get blame on it like these three people are behind this change or these two people are behind this change but it's better to share the responsibility there first this one is definitely for open source maintainers so the title here is in reference to a quote which I don't know where it came from originally but code it says something like code reviews sorry I'm getting my words next up the quote says something like contributions are like puppies everyone likes puppies but you need to look after them and similarly with code when you are given it you need to make sure it's documented you need to make sure it's testing and you need to make sure it works forever so as a reviewer or a project maintainer you should really feel like you can quite happily say reject the change just to say no sorry I can't maintain this I don't have bandwidth to maintain this there was a case when I was working on so one of the projects I maintained the change came in it looked reasonable it wasn't very big but I said to the author sorry I don't think this fits in the project at this time and the response was something along the lines of all I'm asking you to do is just merge it and I said no actually you're asking me to make sure this is something that works until we're able to remove it from the project and that can be quite difficult in open source projects because you never really know what people are using it's really important that everyone reviews I think it can be people would assume that better for just the seniors to review in a project and then the juniors are just writing the code maybe you feel like they can't be held responsible for the reviews and this is actually technically true there was a paper and I have lost the reference I have a paper copy at home I couldn't find it online basically they found out while senior reviewers were doing a better job at finding defects and giving feedback the best way for juniors to actually level up and become more useful and become seniors essentially what to do code review so you need everyone to do it and you've also got to remember there's a whole lot of benefits coming on so there's a share understanding and knowing what's going on if your juniors don't know about this then they're all going to stay juniors I think and the other thing about if you ever have a split in between who reviews and who doesn't you can find that that can create the divide between the two sides in this it can become more of a competition against each other so just make sure everyone is included in the process and so one of the ways you can join in more with an open source project and you can review if you're then also submitting changes maintainers will absolutely love you if you just review a few other open requests or patches whenever you submit a change so with everyone reviewing it's really important to keep them all on the same page it can be a very frustrating process for a contributor if you submit it you get one review and then you get another review from someone else and maybe it takes you back to where you are originally amateur it's just really try and make sure you've got a shared understanding of what is expected from your contributors you can do, you can make this clearer with review guidelines you can also have things like review checklists that your reviewers are all using and following as a guide but you've got to be careful with checklists because you really have to make sure that you avoid anything that can be automated there are a number of things that you can automate in code reviews and by this I mean making sure that your code style passes whatever linting you want to use and you can also make sure that your tests are run and it's just if you as a reviewer are having to do these steps manually you're wasting your time and it's fairly easy to set these things up with something like Travis which is free for open source projects or there's tons of open source tools which you could run yourself as well and one of the best things about automating this is if you run Pyflakes or pep8 against the code and you get a negative feedback there it's much better if it comes from Jenkins or from an automated CI tool rather than from a human if you find that you have your reviewers actually manually commenting on code style then they're just wasting their time and it's far more nitpicky and just drives everyone crazy so the nice thing about using an automated tool is you can actually have a shared hatred of that tool for nitpicking all of you but really the automation is about removing the bike shed so for these kind of discussions where people might debate code style, if it's not tested automatically then ignore that comment as far as I'm concerned if you want to do stricter style guides about something then add a new like a flake extension or plug in something like that and if you're not familiar with what I mean by bike shedding I won't explain it right now but just go to bikeshed.com I think people will debate the simplest details rather than focusing on the actual real problem at hand and I think someone's picking a colour already so moving on there's not much to say about this I have a slide for it just because I think some people will assume it's maybe one reviewer per change but really I think you should consider or at least try multiple reviewers in OpenStack we have many reviewers but a minimum of 2 so that's I find it really useful and I think it really multiplies the benefit from some of these side effect goals that you get from code review about understanding what's going on and one of the other things that's nice about multiple reviewers is you actually actually that's a slightly sad point I'll come back to it when you're reviewing it's important to make sure you're feeling fresh and that you're doing so you really want to do frequent and short reviews there's a paper which is the impact of code review coverage and code review participation on software quality and they found that code reviews peak at around 200 lines per hour a bunch of other papers seem to reach about 400 lines of code an hour is what they suggested so it seems to vary a bit but it was lower than I expected but the important thing is just to make sure you're time boxing this and you're not sort of driving on when you're really fried because you just won't be doing anything constructive personally I find my best time for code review is just after I started drinking my strong morning coffee and I don't know I'm sort of ready to go and I've not looked at my email yet so I'm not very distracted and I'll spend maybe an hour also doing a review then when you're giving your feedback in a review make sure that you are being constructive and you're giving sorry you're giving constructive criticism and you're also giving praise to people and reviews it's very easy to get tied up with giving bad feedback to people as in saying pointing out all the problems it's really nice if you can point out to something that you learned in that code review and I think it really helped make the process a lot more positive interestingly I was when I was practicing this talk a few weeks ago one of my coworkers I got distracted by email notification and one of my coworkers has actually said something on my review I'd submitted he was like hey I learned something today I didn't know you could have a class with just a dot swing you don't need the past thing as well and I just made me feel quite good about it so then I was more encouraged to try and help share more knowledge in the future and this kind of follows on a similar line make sure you're being polite and aware of the tone when you're talking in code reviews this can be tricky and it's very easy if you're in a rush to misword things so in this example if I was saying this to somebody I said hey why didn't you do that that sounds reasonable because you can hear my tone I sound fairly relaxed but if it's written down it sounds much more like why didn't you do that you know it's a I'm not very good at a mean voice but you can get the idea so you can alternatively maybe say something like could we do this instead you know it's just trying to be a bit nicer about how you ask things and it'll make some of the negative feedback easier to receive it's important to make sure you're asking questions rather than telling so worse than either of those examples you would say something like you should do this and just tell them they might have had a good reason for doing that maybe you just missed it so you should always ask and this one I hope is really obvious but you should never be harsh and never personal in your code reviews I think this is probably less of a problem in the corporate environment or a work environment because this tends to be a good social dynamic between people or at least a social dynamic between two people and there's always like managers trying to sort out any problems there's definitely more of a problem in open source communities the python packaging authority has a good code of conduct for basically for projects in general that covers code reviews so I'd encourage that you use that I found out about it because I was I seen the cookie cutter of the python project for using it I do plan to add it to all my projects but I'm not quite got there yet so yeah it's kind of like writing code is hard reviewing is also hard really trying to help out the authors of the chain so again it's helping them help you and that kind of brings together the two different sides that I have in this conversation is about helping each other to come out with a better result and working together automate what you can to save time and reduce that burden of doing automated tasks yourself and be kind to yourself by time boxing your code review make sure you're restricting how long you spend doing it something I missed from earlier but I just remember so I'm just going to say it now one of the real benefits I find with doing reviews is that you can actually learn a lot about a project so quite often when I'm starting to work on a new open source project and I want to make some contributions for some reason I'll actually start doing reviews before I ever submit something I'll be fairly careful about my feedback because I don't really understand what's going on but you can quite often make useful changes and you start to see the benefits of that shared understanding and shared learning in the process so it's a great way to become familiar with the project is to look at the reviews of what's happening and see what the actual, the current reviewers are saying and see what they're expecting from other people and also it's incredibly useful if you can start reviewing like that as well when I told people about this talk a lot of them sort of asked me was I going to do a comparison of different code review tools like what was the best thing to do personally I find that it's not very interesting if you want to look at screenshots or read the documentation about these that's fine and really I'm only qualified to talk about GitHub and Garrett I use them extensively but I know of the others and I'm sure there's a whole bunch more I don't know about but I've never really used them the one thing I think you could really expect from your code review is to make sure you're reviewing before the merge so rather than say committing directly to master in a GitHub repo or sorry in a Git repo make sure that you are having somebody review before that happens so that could be in a pull request on GitHub or in a sort of a Garrett code review for example the reason for this is the feedback loop the feedback loop is just too delayed if you try and review after it's been merged and the effort to actually take this feedback and make updates is just much harder so you'll find that people will do the review and they'll say oh we should have done all this but then eventually it gets forgotten about and no one does it so you need to keep people motivated and only let the code in after it's been updated for this feedback but to quickly talk about the root most people here I imagine probably know so I'll say very little about it but it's got a very loose and undefined workflow the labels are kind of useful for like triaging and things but it's actually quite hard to do any kind of detailed or complicated workflow in your code reviews it's also got a simple UI which works quite nice but some things would be very hard to do for example if you wanted to consider doing multiple reviewers you'd have to have some kind of strange convention to make sure two people had looked at it and it just wouldn't really work that well but yeah everyone has GitHub which is a great benefit and it's also very easy to use and quite appealing as well Garrett on the other hand is pretty much the opposite to GitHub it's open source it's kind of ugly it's hard to use but you can have very defined workflows and rules in it and you can do like much more things like you can have multiple reviewers for example when people do a reviewing in Garrett they'll explicitly give a vote and write your comments and then you'll pick like minus two minus one plus one plus two well they actually they're fairly obvious that they're negative and positive but the nice thing about that is you can actually give some feedback which is like oh I spotted this typo but it doesn't really matter I'll still give you a positive review but if you happen to be updating this for some other reason it would be good to take that into account so it allows you to give the small feedback what the review just because of that one of the really interesting things about having all of this data attached to Garrett is there's a huge amount of information out there online and this is one of the things that I would quite like to do at some point so this is kind of a I'm looking for like-minded people I suppose in OpenStack for example there are around 330,000 reviews in the Garrett but the number is going up all the time obviously and you can pull down all this data it gives you like 20 gig of JSON or something but I'm sure there's something we can learn in there and maybe we can have some kind of feedback loop and there's a whole bunch of other projects are using Garrett in the Open so like Android uses it, WordPress uses it so maybe we could I'm sure there is something to be learned there or you could just have fun looking at the data and making pretty graphs yeah I think it would just be it would be quite an interesting thing to do yeah and that basically wraps up my thoughts about Code Review I'm happy to take questions, these are my contact details and sort of related to we're going to do an OpenStack OpenSpace tomorrow so if anyone's interested more about the OpenStack Code Review workflow they could come and ask there or just plug me afterwards yeah, thank you one thing to note if you're looking at that the O in Google is a 0 I should have picked the mono one for this one hi there great talk I wanted to know as a beginner who's working on a project alone what's a good way to find people to review my code that is a very difficult problem it's kind of the eternal problem of say an open source project is how do you get more people to look at your code how do you get more people to use your code I had wondered if there was some way we could have like a way of networking people that are working sort of as individuals and we kind of have a loose agreement to try and review each other's code that could be quite interesting and it would also get more people involved but yeah I don't really have a good solution there all right thank you hi, I wanted to ask you do you think that reviewing kind of meta issues and like your rejecting merges in terms because because the commit messages are not clear enough or the commits are not squashed is kind of being too picky or it makes sense I think you just need to basically establish some clear guidelines in your project so in most of my side projects I wouldn't be too picky about commit messages but in OpenStack people actually and that's one of the nice things about Garrett is your commit messages actually a file that comes up for review so you review your people review commit messages as well I think there's definitely some value in it but it depends how good everyone is with their sort of keeping their history nice and clean hi, thanks for the talk I also happened to work on OpenStack so I was wondering maybe it's just me who misses in the review comments for instance there's a large patch set that's in progress for a review and there's interesting discussion, deep analysis there's one interesting comment patch at 25 it's a long paragraph and then it's just buried in there on a mailing list based patch workflow you can link to a specific large comment however in Garrett it's just buried and sitting in there like six months later if you want to refer to it again you have to link to the review and then there's like a whole bunch of 25 CI jobs listing their status so it's just very messy how do you handle that? I mean this is a specific problem with Garrett I think so for people who don't know when you submit a change to Garrett someone then does a review and then you submit another change the comments they added on the first revision are hidden so sometimes you might actually do another review before you see the comments so you need to actually go and look for them or spot the email notification or something yeah I think it's just a problem with the Garrett's UI it needs to expose this better all your comments are in draft which were never submitted and I've got a whole bunch of review comments which I thought I had submitted that are just sitting there I found this last week so that was a bit of a nightmare I was like that's why they didn't listen to that thanks it's more for wine I guess yeah any other questions? so I was just wondering what your thoughts are on code review versus pair programming yeah so this is quite an interesting one I think you so I work remotely for Red Hat which means pair programming is a slightly different thing and that we sometimes do it and sort of sharing a screen terminal and maybe a voice chat at the same time and that can work quite well but it means I've not done a whole lot of pair programming essentially I think that you can basically get the same result out of the process but if you can so in open stack at least you still need to have other reviewers just for the simple fact that the change won't be merged until you have enough reviewers so if you pair programming it doesn't necessarily help you out in open stack but I can see that in some situations you could just replace code review with that I mean if there's only two people working on a project then there's not much point doing a formal code review if you're doing pair programming I think the result could be the same yeah it would be interesting to know if anyone's got experience like comparing the two thanks just a short response to your question because where I work we have a sign-in for code review per week so before we start working on a new feature for example we come together and discuss the implementation so you avoid having feedback after three days of work saying yeah this you don't agree with your implementation or your choices so that I found it to be really useful to work on a feature together in the way that you plan it together and discuss it before you actually submit your pull request that works very well I think again this is maybe an artifact of the fact that I'm a remote worker that what I quite often do is I will put my code up for review very early on and in Garrett you can mark it as work in progress so that allows people to have sort of a rough look at it from a high level but not do a proper review and then you can maybe get some feedback a bit earlier which is another good approach to that to sort of avoid wasting say five days or three days development but yeah if you're doing sort of scrum meetings it's always a good time to bring up these kind of questions as well as well anyone else thanks for your talk did you maybe try to implement trunk-based development do you have experience with that and code review so trunk-based development is just when every change goes straight into everything goes to the master small changes all get that's essentially what happens in OpenStack every change goes directly into the master branch unless you are back porting to a previous release something like that so yeah I guess we do do that and it seems to work pretty well I've never really been a fan of the long running branches that they cause more problems which I guess is the alternative I'm not sure if I'm missing something else about trunk-based development okay and what size are your average commits like in lines of code that's a good question if I look at this data in OpenStack I could tell you I guess I like them to be a couple of hundred lines at most occasionally if you're moving things around you can end up with really big changes but that tends to be a bit easier to review yeah I'm not sure I would have to look at that and see Thanks Hi, thank you for the talk I wonder if you have some experiences or ideas or comments about reviewing different things than code like I don't know in OpenStack we have blueprints and specifications and sometimes code style changes and so on yeah I think it can be good but you've got to be really careful and this is a problem I think is happening in OpenStack in some projects when you want to make a change there's basically a repository with just restructured text files and you submit with a document describing what you want to do but the feedback processing that tends to take a really long time because people really start to go into far too much detail on the specs so I think that tends to not work so well for some reason documentation I guess is a bit more like code and that seems to be fine because you're actually documenting what something does whereas when you're reviewing the spec a lot of people are trying to interject how they think it should work and it almost gets to the point where people are debating the implementation and to satisfy the code reviews that you're getting you almost need to write out pseudocode for everything you want to do and it just becomes a big duplicated effort and I think that's why we've seen a lot of OpenStack projects move away from this really detailed spec process or at least they've got something more alternative for smaller features Hi, great talk by the way I just wanted to ask you something you stated and I totally agree with you that code review is not it's not just a one ping-pong thing but it's just a dialogue a continuous dialogue but where do you draw the line how far do you go with this dialogue in order to either reject a pull request or merge the pull request I guess it depends on the situation you have to really define it based on your project if you're a sole maintainer of the project you can simply think do I want this change do I want to maintain this change and then draw the line there but if you're working in a company then you're probably more thinking along the lines of what does the business need you've got to put on your community hand think what does the community want does this match the community's expectations it might not match what you want exactly from a feature but maybe more there so there's not really a simple way to do that I don't think you've just got to make sure you're taking into account the group that you are working with Thank you, Dougal so that's lunch don't forget about the massages