How do you build reliable software with fewer bugs? Yes, unit testing is part of that.
But did you know that code reviews often play a key role in this process
and come with many more benefits on top of just bug detection?
This week, you'll meet Dougal Matthews. It's Talk Python to Me, episode 102, recorded February 28, 2017.
Welcome to Talk Python to Me, a weekly podcast on Python,
the language, the libraries, the ecosystem, and the personalities.
This is your host, Michael Kennedy. Follow me on Twitter where I'm @mkennedy.
Keep up with the show and listen to past episodes at talkpython.fm
and follow the show on Twitter via at Talk Python.
This episode is brought to you by Rollbar and Hired.
Thank them both for supporting the show.
Check them out at rollbar and at hired underscore HQ on Twitter and tell them thank you.
Dougal, welcome to Talk Python.
Hi, thanks. It's an honor to be here.
Yeah, it's great to have you here.
I'm really excited to bring a slightly different view of technology here.
Often we talk about, you know, how do you execute this code?
What's the API you call?
But let's take a step a little bit back from just writing code
and talk about how we work as a team,
how we contribute to open source,
contribute to our company's code,
and things like that through this code review
whole conversation that you have here.
Yeah, I mean, I spend so much time doing code review
just as part of my daily job.
It just seems like such an important thing to actually talk about
and see how we can improve.
Yeah, and I feel like people don't talk about it very much.
That's why I was drawn to a talk that you gave at EuroPython
that we're going to talk about is,
you know, it's great that you're talking about this here
because it can actually be either very beneficial
to bring in people, kind of to level them up as developers,
or it can be confrontational and actually upsetting, right?
So I think having this as a thing you study
and a thing that you do meaningfully,
not just, hey, I have to review your code, somebody said so.
Seems like a good idea.
Absolutely.
At the EuroPython talk, I actually,
I'd done a quick sort of raise of hands
to see what people thought about code review.
And I think it was only around 20% of the room
said they looked forward to or enjoyed the code review process.
And given how long, how much time people spend doing code review,
which was several hours a day, if not more in some cases,
then that's just something we need to do better at.
Yeah, we definitely do.
Before we get to that, though, let's start with your story.
How did you get into programming?
Yeah, I've got a fairly common story.
I think I started messing around with computers when I was young.
The first thing I'd done really was learning sort of HTML
and I guess CSS a bit later on,
just because I first encountered it in some websites
and I looked at the view source.
And just with that, it kind of led me down this rabbit hole.
My dad had actually done something really great
when I was a teenager.
He embraced it.
He was running his own company at the time.
And not many people actually had websites
for their small businesses.
What year was this? Do you remember?
It must have been around 2000.
Okay, yeah.
Maybe a couple of years before.
Yeah, I mean, the web was certainly well established.
Like the whole dot-com boom was going crazy around that time.
It was still not a thing that if you had a business,
you had a website, right?
Like that was not a one-to-one mapping.
It was if you're doing internet stuff
or you're a big company like Coke or Chevrolet or something,
you would of course have one, right?
Yeah, absolutely.
I think at that point,
it was really out of reach from your regular small companies.
So yeah, my dad just basically got me to put together
a simple website for him.
And honestly, it was fairly terrible,
but it was my first kind of step into that world
and that direction.
And I just, I love the idea of web development since.
Yeah, I think it's interesting that the whole view source thing
brought so many people into programming, right?
It used to be we would go to a store,
we'd buy a box,
it would have compiled machine instructions on a disk.
We'd put those on our computers and run them.
And then the web came along and if it did something interesting,
you could pretty much just look and study it, right?
Yeah.
And that actually reminds me of one time,
I was just starting and I was trying to figure out
why something wasn't working.
And actually, I didn't have access to the computer all the time.
So what I'd done is I printed about 20 pages of HTML
and just sat down and studied it.
It wasn't a very good approach, but...
I've tried that as well.
It's extremely hard to get anything out of it.
I did it with C++.
Awesome.
Okay, so you created the site.
What language was that in?
It was purely static.
So it was just HTML.
I'm not even sure if I was using CSS at the time.
It might have been a bit early in my learning at that point.
But fairly quickly, I moved on to doing PHP.
I think it was just because you've seen it everywhere.
All the websites ending with .php,
like on, I guess, Facebook later on.
But they were everywhere at the time.
I never really felt quite at home with PHP.
It wasn't really the language for me.
I'm not really quite sure what it was.
I mean, it was just a large number of things.
I guess it always felt a bit strange.
And I was trying to do more.
Like, I was trying to do more object-oriented
and think about my code more from an engineering point of view.
And eventually, I stumbled on a video by Jacob Cavill Moss
when he was introducing Django.
It was very early on.
It was before Django at one point.
And I just picked it up and tried it out.
I just loved it.
It was great.
And that just kind of led me down that path to learning Python.
I remember writing a blog post years ago that Django,
it's just Python because it was Python that I really liked.
I mean, I do enjoy Django, but it was Python and the full ecosystem
and everything beyond that.
And it just kept on going.
Yeah, I hear that's a story sort of like that over and over
in that the guys around Django were very just enthusiastic, right?
And their enthusiasm wore off on anybody who came in touch with them.
And Django just really grew, partly because it's really great,
partly because it's Python,
but partly just the enthusiasm of getting started with this new world, right?
Yeah, absolutely.
And I'd gone from sort of hacking on WordPress sites
and various PHP frameworks to then suddenly the elegance of Django
and Python just defining models and having the admin
and all that just there was just amazing, mind-blowing, really.
You mean there's objects in architecture and factoring?
Oh, my goodness.
Yeah, yeah, very cool.
So you said you have some strong ties to Europython.
What's the story there?
Are you working with the conference somehow?
The reason I wanted to mention Europython,
I mean, there's actually several reasons.
But when I started with Django,
I just thought I need to get involved with this.
But there's nobody local that I know.
I can't find anyone that's doing anything with Python or Django.
So as a poor student at the time,
I found all the money I had and got myself to Europython in 2009.
And that's the first Europython I went to,
and I've been every year since.
And I also went to the first and actually the only Euro DjangoCon
because after that I got changed to DjangoCon EU.
Yeah, and it's just I've met so many people there
and I slowly have started to do some small things to help out with Europython.
So I'm on the program committee this year.
And I'll actually take the moment to point out that the Europython dates for this year
are the 9th to the 16th of July.
These are tentative, but I expect they'll be finalized by the time this is released.
Oh, yeah, that's really great.
Thanks.
So if you're anywhere near Europe or you want to go to Europe, right?
Check it out, right?
What's the URL?
I think it's just...
It's like Europython.
Europython.eu is the website.
Yeah, that.eu.
And this year it's going to be in Rumini in Italy.
Oh, that sounds awesome.
Okay, excellent.
Well, there's a possibility I'll make it there,
but it's very far away from it.
So we'll see.
But that's awesome.
If people can make it, they should go.
I think the Python conferences are something really, really special.
There is a different way that people interact with each other there than a lot of typical
tech conferences.
So yeah, if you can get there, check it out.
Yeah, I agree.
There's a long-running phrase of come for the language and then stay for the community.
And I think that really resonates with me.
Yeah, absolutely.
So you're doing a lot with Europython, a lot with Django.
What do you do for your job today?
So I'm currently working on OpenStack.
I'm employed by Red Hat and I work on the OpenStack project full-time.
So I'm focusing primarily on making OpenStack easier to use and install.
And I'm also working on a workflow service within OpenStack, which is quite an interesting project.
I think that's really good.
OpenStack is really a great example of Python doing major stuff.
And it's a great contribution, but it is a little bit hard to install,
especially if I just want to run it natively on my Mac or something.
It's challenging, right?
Oh, absolutely.
I mean, I think the problem with OpenStack or one of the problems with OpenStack is
it's such a wide range of different projects.
There's no two OpenStack installations that are actually the same.
Some people care about containers or some people can care about VMs.
Other people can care about storage, distributed storage, that kind of thing.
They're all very different.
Yeah.
I mean, you're basically installing AWS in a sense, right?
Yeah, exactly.
Which is quite the thing.
And by the way, just S3 is entirely down and the internet is,
is in chaos today.
So that's quite something.
I'm really glad our recording is working.
That's great.
All right.
So let's talk about code reviews.
So first of all, let's start with who should be doing code reviews.
Like, are these big enterprises?
Is this open source?
Like, who should care about this?
I think everyone should be doing code review.
Really, the only reason I can think of to not do code review it, and that would be that you
are the sole developer on the project.
And obviously, then it's kind of tricky to have a peer review.
Most of the time, I'm involved in open source projects and that kind of environment.
But I think code review is something that should be in the enterprise and all companies as well.
And I think, actually, it's probably more common in open source just because of the nature of how people communicate and collaborate.
Yeah, okay.
So I tend to agree with you.
I mean, I guess if you're on a small team, you work at like a small company, and you have one project, maybe there's some other developers, but they have their other project, and you don't really cross over.
It might be a little bit tough to do code reviews, especially if one guy is a C++ person and another person is a Python person or something where they don't know deeply what they're looking at.
That's a fair comment.
But you're missing out on an opportunity when you're looking at situations like that.
Perhaps the Python person becomes ill or leaves the company if nobody else knows what they've been doing, then you're in trouble.
At least if they've had a light code review process, they might not need to be as in-depth as in some other situations.
They will have some idea of what's going on.
Yeah, I think that's a really good point.
You also actually pointed out some scenarios where it might make sense, even as a solo developer, to do your own code reviews.
Yeah, so this is kind of an interesting thing, which I just stumbled on by mistake.
I do some work on one of the documentation projects used in Python.
It's called mkdocs.
And it's a fairly popular project on GitHub.
There's quite a lot of people watching the repo.
And I was just hacking away on it.
And I opened a pull request onto the project just because I wanted to trigger the Travis build and make sure things passed and were working before I merged it, just to avoid myself making any errors.
But in the process of doing that, I left the pull request open for maybe six hours or half a day.
And somebody came along and actually made a comment on it and made a suggestion.
So it's just by including that process in something that I'm working on, even if I'm planning to just merge it myself anyway, it gives other people an opportunity to get involved.
And it actually helped me find new contributors as well.
Oh, yeah, that's really cool.
So especially if you have open source.
But really, if you have a repo that other people are watching, going through the whole workflow, even if it's just a cursory sort of review, lets them know there's like this milestone, like a feature is fixed.
And here's the code that fixes that feature, right?
Yeah.
And also, if you have your CI integrated in that, it will avoid you making any sort of silly mistakes where you've accidentally missed a file or added a file because your test will run against that pull request as well.
Yeah, yeah, absolutely.
So do you follow or subscribe to the GitFlow style with feature branches and stuff and PRs?
How do you guys work together at OpenStack?
We basically do master development.
Every patch goes into master and then we've got stable branches for releases that are still receiving updates.
It's a much lighter workflow than something like GitFlow.
I find that GitFlow might work well in a company where you can force everyone to follow the same conventions easily.
But in an open source project, I don't think long lasting branches tend to work very well because they just get out of sync and there's different problems going on or they get forgotten about.
So I prefer everything just to go into master as soon as possible, but keeping master as stable as possible as well.
Yeah, I think that's a really interesting point.
And I've worked both ways.
I've worked in a company where we basically agreed to do the GitFlow stuff.
So every change has its own branch, like a feature branch or a bug fix branch, and then you do a pull request back to the dev or main or whatever to have someone else review it on the way back in.
But I've also worked in the – basically, we've got a dev branch and a releases branch.
And we just push, you know, merge over to the releases every now and then.
I don't know.
I've torn.
Like they're both – they both have their advantages.
One of the things I see about the second one, the latter one, like what you described you're doing is the closer you are to dev on your branch or what you're doing, the less merge pain you're going to have.
Like a friend of mine describes sort of being branched as credit card debt.
A little bit of credit card debt is fine.
If you let it build up for six months, there's going to be a reckoning when you try to fix it, right?
Yeah, absolutely.
And I think if people are using longer-running branches, then as long as you're rebasing, then you're maybe avoiding most of the problems.
That's right.
So if you take the dev branch and you merge it into your feature branch every now and then to like keep – basically to keep that time, the work that's been done from the time that you're working on your code to what you're going to check in minimum, that's great.
Okay, so in my mind, I guess code reviews are obviously about finding bugs or making sure that you follow certain procedures for, say, security reasons.
But you actually said there's a lot to do with like code quality and mentoring and a lot of things.
So like what are some of the benefits?
Yeah, so this is actually quite an interesting point.
There's a paper which is called Expectations, Outcomes, and Challenges of Modern Code Review.
This is one of the first academic papers I read about code review, and it's kind of what led me down this rabbit hole.
At least I hope I'm remembering the right paper.
This data might be from another one.
We'll put all the papers, the links to the papers, and people can find the references.
Somebody had done some research with Microsoft, and what they'd done is they'd done a questionnaire for the developers before and after the code review process.
And they were just finding out why they were doing code review, what they were expecting to get out of code review, and the motivations.
And while most of the developers actually stated their primary motivation was to find bugs and find problems, when they took the outcomes of code review, so they looked at the comments and all the different feedback that was in a code review, then they manually classified it based on all the different things that were coming out of it.
They actually found out that bug finding was much smaller.
While bug finding was still important, it was a much smaller piece of the overall code review process, and there were a lot of other benefits that you'd get out of it as well.
I see.
Yeah, they probably started out the same sort of mental model, like, okay, how well did we do finding bugs?
And like, oh, wait, all these other good things came out of it.
Wow.
Yeah.
Yeah, and I think this is part of why a lot of people don't enjoy code review is because if you're going in just to find bugs, you're kind of going in with a negative stance.
You're thinking, what can I find that's wrong with this?
What can I find that's broken?
It's not.
It's a slightly different process.
But what they actually found was the biggest outcome was a code improvement.
So this is kind of subtle to a bug because the code that was up for review was valid, and it worked.
But actually, based on the feedback and the iteration that went on in the code review, they ended up with a better solution in the end.
And then there's understanding and shared knowledge between the teams.
So it's about having awareness of what other people are doing on different projects or different areas of the code base.
Okay.
Interesting.
Yeah.
So you had some interesting quotes, like one from Jeff Atwood, the creator of Stack Overflow, about the value.
And it's also one from Steve McDonnell, right?
So yeah, Jeff Atwood, who a lot of people will know, he wrote a blog about code review a while ago.
And he basically said that the hardest thing about doing code review is finding somebody that you trust enough to do the code review with.
But once you actually get into the process, you'll find that every minute you spend in code review is paid back tenfold, which is a really huge sort of payback for your time investment.
Yeah, that sounds like a good investment.
And then the one from Steve McDonnell.
So he was basically, in his book, Code Complete, he compared the detection rate.
So how often you're finding bugs in your reviews.
And in their study, they found out that unit tests had a 25% chance of catching bugs.
Function tests had a 35% chance.
Integration tests, 45% chance.
But then code reviews were actually between 55% and 60%.
So I'm not sure I completely buy those numbers.
But I mean, just even think that code review is just as valuable as your unit tests is quite something.
I mean, I couldn't live without my unit tests.
Yeah, it certainly is interesting.
And that book, Code Complete by Steve McDonnell, it's really, it's quite a book.
It really has a lot of wisdom, a lot of things like backed up by studies and numbers.
And so if you haven't seen that book and you're kind of into this process stuff a little bit, definitely check it out.
So the fact that you get better bug coverage than unit testing, that's pretty interesting.
Also, how do you feel like unit testing contributes to mentoring and better understanding and sort of team understanding versus, say, like code review?
I feel like you would get more of that out of a code review, right?
Because you're actually talking, not just writing tests.
Yeah, absolutely.
So one of the things I covered in my talk was the issues with the sort of the quotes that I gave is they are very focused on bug detection and defection rate.
And as you said, there's a lot more to code review, which I cover as well.
I think the reason that paper is covered in the bugs is bugs are sort of an absolute thing.
Something is a bug or it isn't.
So it's very easy to measure.
You can see how many bugs have been reported, whereas all of the other benefits, they tend to be a lot softer and a lot harder to actually get a tangible measurement of.
Yeah, like that example of what if the Python guy gets hit by a bus, right?
Your company is more anti-fragile if you do these types of things, right?
It can withstand these problems, whereas I guess it's obvious, like the user can't log in and you get a 500 error on the server.
Okay, well, that's a bug, obviously, right?
But you can say, well, we could have missed this with, we could have saved this with a test.
But also, like, our team works better together.
Our team has a better understanding of what each other is doing and the tradeoffs they're making and so on.
You can't really test for that.
No, not at all.
This portion of Talk Python to me has been brought to you by Rollbar.
One of the frustrating things about being a developer is dealing with errors.
Ah, relying on users to report errors, digging through log files trying to debug issues, or a million alerts just flooding your inbox and ruining your day.
With Rollbar's full-time error monitoring, you'll get the context, insights, and control that you need to find and fix bugs faster.
It's easy to install.
You can start tracking production errors and deployments in eight minutes or even less.
Rollbar works with all the major languages and frameworks, including the Python ones, such as Django, Flask, Pyramid, as well as Ruby, JavaScript, Node, iOS, and Android.
You can integrate Rollbar into your existing workflow, send error alerts to Slack or HipChat, or even automatically create issues in Jira, Pivotal Tracker, and a whole bunch more.
Rollbar has put together a special offer for Talk Python to me listeners.
Visit Rollbar.com slash Talk Python to me, sign up, and get the bootstrap plan free for 90 days.
That's 300,000 errors tracked all for free.
But hey, just between you and me, I really hope you don't encounter that many errors.
Loved by developers at awesome companies like Heroku, Twilio, Kayak, Instacart, Zendesk, Twitch, and more.
Give Rollbar a try today.
Go to Rollbar.com slash Talk Python to me.
Yeah, so how do you feel like automation fits into this?
So like continuous integration and whatnot?
So I really strongly believe that you should automate as much as possible.
Anything that can be automated that isn't is essentially wasting your time.
And for most people, their time is the most precious thing.
So this means that you should be automating your linting.
So this is like your PEP 8 checking or your FLAC8.
Yeah, you had an interesting quote where you said something to the effect of this whole thing often gets set up as an adversarial sort of relationship.
Like, okay, these guys sit on the side of that table.
You sit here.
We're going to review and tell you what you did wrong and how you need to fix it.
And one of the things that code reviews are supposed to do is like say, well, you're not following this coding convention or you didn't do this thing.
You know, you didn't document this or name this correctly or whatever.
Right.
And if that comes from a human, it seems super nitpicky.
But if it comes from a machine, everybody is sort of subjected to the thing absolutely uniformly.
So you're like, oh, yeah, FLAC8 told me that should have an underscore and not camel case or whatever.
Right.
Yeah, absolutely.
The sort of the feedback from an automated machine.
So in terms of also including verifying the tests are running, it doesn't feel negative.
It doesn't feel like the code is judging you.
But sometimes if somebody comes along and reviews something that you've been spending a lot of time on that, but then they just point out different small little things that they think would be slightly nicer formatting, then that can be infuriating.
Yeah, I think it's infuriating, but it also can't be anyway.
But I think the other part that is there is if your job is to be the reviewer, you don't want to spend your time going, yeah, you named that wrong.
Yeah, that needs a comment.
Yeah, that needs a doc string.
Like that is not that is like a job that robots could do and robots should do.
Right.
You should say like, no, no, I think this algorithm is when we scale up, this is not going to scale or this is a stateful thing you're doing.
That means like we can't load balance it.
So we need to do that different.
Like that is the kind of thing people should be contributing to and not spending the energy on camel case, camel case, doc string.
Yeah.
Right.
Yeah.
So focusing your time on the design and the architecture and the actual solution to the problem is just so much more productive.
Yeah.
And so you said you were trying to rename this process to like something other than a review, right?
Yeah.
I have a bit of an issue with code review.
I think review is a slightly negative feeling about it.
If you think of something like a performance review at work, that's rarely a good thing.
And your manager says, okay, we need to have a performance review.
I much prefer to think that it's going to be a code collaboration or a code discussion.
It's something that you're working on with someone else.
And I think we should try and encompass that more.
I don't realistically think we'll ever change the name at this point, but that's how I perceive it in my head at least.
Yeah.
That's cool.
I think code collaboration.
To me, maybe code conversation, right?
Something like this, right?
Yeah.
That's awesome.
Yeah.
I mean, certainly in the open source space, I feel like these can almost be a conversation starter or an introduction to somebody maintaining an important project.
Like you can go look at some code, fork it, do some work, and then your pull request like starts the conversation.
Like here's how I think I can both contribute to your project and improve it.
And then, you know, you get to know that person a little bit, at least by going through and reviewing their code back and forth on, well, you didn't think about this in our project and you're going to have to account for it and so on.
Right?
Yeah.
And I think it's really important when you submit a change to a project that you state that you want to have feedback.
I just want you to merge this.
And I was like, well, you've only just opened this pull request.
We're only just starting to talk about it.
I've never seen this before.
So this is like the start of the conversation.
And I think both people need to go into it at that point.
Whereas sometimes somebody's maybe been working for a week on something and they feel like when they opened the pull request, they kind of feel like they've done.
They feel like they've finished.
But that's not the case.
Yeah.
No, it's the beginning, right?
The beginning of the conversation.
What would you recommend to have made that person's experience better?
Should they have opened an issue and said, I'm going to do a pull request.
Here are my intentions.
Here's the problem I've identified and what I think I'm going to do.
I'm sort of personally torn because on one hand, actions speak much louder than words.
Like here is the code that I have written that will fix this for you.
Puts you in a different place than somebody who opens an issue and sort of like almost complaining.
Right?
So they come with more credibility, but at the same time, they put a lot of energy into it.
And they may have gone in a path that was incongruent with what you're doing.
Right?
So what do you think?
Yeah.
So I think in this case, you just need to judge it yourself.
Really?
There isn't a standard answer.
Sometimes what you're working on is fair.
It seems fairly obvious.
So it's fairly small and contained.
And then you should just go ahead and do the pull request.
But if you feel like there's maybe more discussion, you're not sure about how it should work, then it's better to go towards an issue first.
I do think that sometimes it's actually easier to express what you want to do in code.
Like you're just messing around.
So I think it's also fine to just go straight to the pull request.
But you should just always have the expectation that this is a very first draft and it will change or it could change.
Yeah.
I think maybe think of it as like a draft of a paper sort of thing.
It's a pretty interesting way to think about code.
Right?
Okay.
So when you're authoring changes, don't start with a code.
Well, what should you start with?
You should either be starting.
Well, you can start with the code if you want to, but it's always good to start with a discussion first on an issue or sort of a bug report somewhere where you can actually flesh out what it's going to be.
But that isn't always needed if it's going to be a much smaller change in the end.
Yeah.
Also, you know, understanding the context of where that person's coming from, where the project is.
Right?
Like on one hand, you might, it would be very easy to look at one algorithm and go, this algorithm does everything in memory.
It loads it all up, all this data up at once.
And it's going to be super slow.
We can't do this.
Well, that's true if you have a billion records.
If you have 20, it's totally fine.
And the billion record solution would actually make it worse.
Right?
So understanding context, I think, is super important.
Yeah.
And when you actually provide a change, it's really useful to tell people both why you were doing something.
Sorry, what you were doing and why you were doing it.
I quite often see commit messages or pull requests that only really cover one of the two.
And I think you really need to understand both as a reviewer to really give a thorough review to that code.
Yeah, for sure.
I think, obviously, you have to keep that in mind no matter what.
But I feel like in open source, you don't necessarily know how people are using your code as opposed to, like, within your company, like, well, it's that website.
We know what we're doing.
We know what the traffic is.
We know how that's going to look.
Right?
So it seems almost more important in the open source, open space world rather than confined behind your company's wall.
Yeah, absolutely.
So say, for example, you need to add a new sort of method to a function for some reason.
If you just write in your pull request, added parameter A to this function, then that's not very useful.
You need to also explain why you're doing that.
What is the actual advantage to this as well?
Yeah, and how you're not going to break everybody's code by doing it.
Yeah.
Another interesting piece of research that you pulled out, something from Mozilla, was people actually find more issues and give better feedback on small changes, small PRs and so on, rather than large ones.
Even though the large one surely has more things to address.
This happens because reviewers are just overwhelmed by a big change.
And I actually seen someone tweet about this.
Basically, they decided that they should just submit bigger changes because people do less thorough reviews and they get their code in more easily.
Sneak it back.
Yeah, that's obviously not the idea here.
So if you can just keep your changes really sort of contained to one particular thing, one particular issue, and ideally touching less files within the project.
Although, obviously, sometimes you need to touch multiple files.
It just makes it easier for someone to review and get their mind around it and really understand what's going on.
And I think that's why the smaller changes then get a much more valuable review, whereas the bigger changes, they tend to nitpick at different things, but they've maybe not quite been able to wrap their head around everything that's going on in this review.
Yeah, so do you recommend sort of having one concept per check-in type of thing rather than, I did all my work for the day, so I'm done, so I checked it in.
But here's this little change I made, then I reformatted this thing, and then I addressed this performance thing over here rather than just all at one shot.
Would that get better feedback?
I'd definitely go for splitting that up into multiple reviews.
I actually find that you mentioned that you reformatted something in that example, and I actually think that can really make it harder to review something because the diff could be 10 times bigger just because they've also reformatted the code at the same time.
They do that as two individual changes, and you can actually see, okay, this is where they've actually made the functional change.
This one's just laid out.
Okay, they're both fine.
Something like this can depend a bit based on the tool that you're using.
So if you're using a GitHub pull request, if you just have two commits in your pull request, then that would be fairly easy to review in that case.
But something like Garrett, which only allows one package at a time, it's a bit tricky.
Yeah, sure.
What do you think, what contribution to this do you think GitHub has made?
How important is all of this sort of well-known, commonly used infrastructure that is kind of GitHub.com for code review, code collaboration?
Do you think the tools are adjacent?
I think they're reasonably good.
They've actually made some changes over the last, I can't remember when exactly, but I think over the last six months or so, they added a whole bunch of features.
And at least some of them appear to be inspired by Garrett and maybe other code review tools as well.
I think they're improving the process.
Originally, I think it was almost too simple.
But actually, for small projects, it seems to work quite well.
When you see larger projects on GitHub using pull requests as their primary code review, they tend to come up with a very complicated system on top of that with labels.
Or they maybe, I think Facebook has bots on all their different projects that are like progressing things on and sort of marking them as failed or not.
So yeah, I think they've done a great job in making code review accessible to everyone to at least a degree.
But yeah, I still think there's space for improvement there.
Yeah, it's interesting.
I mean, it's not necessarily true that the code that, you know, say you guys are using for OpenStack or Python is using for CPython is necessarily the best same set of tools for people with a thousand line project.
Right.
Like there's overhead in these bigger things that the smaller ones would actually suffer from it being there.
So it's always a tension, right?
Yeah, absolutely.
I mean, so we use Garrett for OpenStack and it's a Python open source project.
So that's quite nice.
In all honesty, it is very hard to use.
It's also extremely ugly and it takes a long time to just get used to using it.
But once you do get to that point, I find it really good.
Whereas GitHub, I feel like that's maybe the opposite.
Whereas it's really easy to use, but you start to hit up on the limitations of it fairly quickly.
Yeah, I think that's totally, totally correct.
So, I mean, even simple stuff as like nested issues or to-dos, right?
Like I'm not even sure if GitHub supports those, but it can be like, well, we want to do this thing, but there's actually five things we have to do to make that happen, right?
That could be important.
Okay.
Now, you talked about how if you're the reviewer, you shouldn't see this as like I'm assigning a grade or judging this person's code, but maybe think of it as like a shared responsibility with the author.
And you had a nice picture of like bunnies or guinea pigs or something.
Yeah, it was a guinea pigs.
I think I found that on Flickr.
Yeah.
I think it's really important when you're reviewing something that you take on ownership of that code as well.
Because effectively, I think the author of the code and the person that's approved and merged it are just as responsible for it ultimately.
This may not work in practice sometimes because people that submit the changes that get merged, they'll disappear on an open source project, for example.
You should really be trying to share that responsibility.
And I think this sort of highlights an issue that you would see with something like Git Blame because you would sometimes use Git Blame to look through the history of a project and see who has written what.
But that will only ever show you the author or sometimes the merger, depending on how you're doing things.
It won't show you everyone that's involved in the code review process as well.
And I think surfacing all these different people is useful.
As a side, I think Git Blame is a terrible name for the command in general.
But I think just having something which helps you find everyone that knows about this.
It's more about finding someone that has the knowledge here rather than finding someone to blame, in my opinion.
Yeah, I agree.
I certainly blame.
While sometimes it feels appropriate, you're like, all right, what is this?
Who wrote this?
It's not a real positive thing, right?
And you're right.
It only captures the final commit, which it could have actually just been the person who just reformatted it because there was a problem, right?
It doesn't even necessarily mean they created it.
Yeah, or moved the code around.
Yeah, yeah.
Something like that.
You also said that code contributions are like puppies.
Yeah, so the picture of the puppy is actually my dog when she was only a couple of months old.
But it's quite a nice quote.
I don't know where it came from, so I can't attribute it correctly.
But somebody said that code contributions are like puppies.
Everyone loves them.
But you need to look after them.
You've got to walk them.
You've got to feed them.
You've got to care for them.
And this is the same for any code contributions that you receive.
You need to make sure that they continue to work.
You need to make sure they're documented and so on until you can deprecate it.
And that often is quite hard to do in open source projects.
Absolutely.
Well, and I think the puppy analogy is nice.
Like everybody loves them and they're cool when they're nice and cute.
But, you know, when they're four years old and it's raining and you're busy, do you really want to walk them, right?
Or do you want to leave the party early because the dog needs to go out?
It's easy to say, well, I just want to change this behavior a little bit.
I just want to add this function.
I just want to like add an overload or a default parameter here and think how hard can that be?
It's like two lines of code.
I'll check it in.
Well, there's the documentation.
There's that video you recorded showing people how to use it that you have to re-record.
There's this thing you have to redeploy.
There's this, these, like they can, the smallest changes can expand in like large ways, right?
So under, like having somebody who knows that, like it's reviewing and going, you don't really fully appreciate the cascading changes that the small change is going to have, right?
Yeah.
And I think it's, it's something that open source maintainers in particular should feel like they can do, they can reject code more basically on this basis.
I think people quite often feel pressured to accept contributions.
Right.
I worked hard on it.
And so why don't you want to accept it?
It works.
It passes the test.
Yeah.
But then they then are responsible for looking after that code for forever.
Yeah, yeah.
This portion of Talk Python to me is brought to you by Hired.
Hired is the platform for top Python developer jobs.
Create your profile and instantly get access to 3,500 companies who will work to compete with you.
Take it from one of Hired's users who recently got a job and said,
I had my first offer on Thursday after going live on Monday and I ended up getting eight offers in total.
I've worked with recruiters in the past, but they've always been pretty hit and miss.
I tried LinkedIn, but I found Hired to be the best.
I really liked knowing the salary up front.
Privacy was also a huge seller for me.
Sounds awesome, doesn't it?
Well, wait until you hear about the sign-in bonus.
Everyone who accepts a job from Hired gets $1,000 sign-in bonus.
And as Talk Python listeners, it gets way sweeter.
Use the link Hired.com slash Talk Python to me and Hired will double the signing bonus to $2,000.
Opportunity is knocking.
Visit Hired.com slash Talk Python to me and answer the door.
You know, I had a recent experience with this.
I suggested some change to pyramid the web framework and they're like, great.
And if you're willing to be the maintainer of this part of that feature for two years,
then we're totally happy to accept that.
Is there a simpler way we can solve this problem?
Maybe.
So they subscribe to the puppy philosophy.
Yeah.
I mean, it's nice that they were willing to give you that opportunity because actually it's
quite hard to get to that point of trust with people sometimes because a lot of people might
promise to support something for two years, but then they'll just disappear shortly after.
That's right.
The other thing that I thought was pretty insightful is you said that not just senior
devs should be reviewing code.
It shouldn't be a top down, like I'm above you.
And so I look down on your stuff and I tell you whether it can be approved, but also juniors
should do review, maybe even a seniors check-ins, right?
This is a mistake which I've seen in a couple of companies.
Not recently, actually.
So hopefully it's sort of a trend that's dying now.
I've seen cases where only the seniors were allowed to review the code and the juniors
would then sort of be subject to the review, but they would never get the chance to review
themselves.
There was actually a study which found that senior developers were actually doing slightly
better code reviews just because of their experience.
They also found that junior developers most quickly improved and most quickly became on par
with the other seniors by doing code reviews themselves.
I think that really highlights back to all the other benefits that come out of code review,
like with the shared knowledge and the shared understanding.
That's how the juniors become seniors.
Yeah, absolutely.
I totally think that's a great idea, a great point.
Sure, they'll find fewer bugs, potentially, because they don't really know as well to look
for them.
Maybe their suggestions will be pushed back upon.
Like, I suggest you rewrite this in a simple way.
Well, no, no, it won't scale and we have a million users, so on and so on.
But even just having that conversation, making them actively think about it and propose those
changes, whether or not they actually make it through, the value has already been gained
from that, right?
Yeah, absolutely.
Okay.
Very nice.
You also said having a review checklist might help.
It seems to me like that could depersonalize it a little bit.
Like, look, these are the things we're going to check for, so we're going down the list
and it's not me being a jerk to you.
This is a list.
Like, it applies to me too.
A code review checklist can be very beneficial because it gets everyone really on the same
page and everyone's kind of reviewing to the same standard and the same expectations.
The one thing you need to be careful with is that you don't include anything that could
be automated because there's temptation.
It's easier to write something on a sort of a review policy than it is to automate it.
So you just need to make sure that that doesn't happen.
But otherwise, I think it's a really good idea for projects to explicitly sort of state what
they're expecting.
And this can be useful both for the reviewers and also somebody that is submitting something
for review because they know what they are working towards.
Yeah, that's cool.
Certainly, it seems like you could just build the checklist and go, okay, what of this can
we automate?
And we're not going to use this checklist until we've gotten everything that can be automated
automated.
Yeah, that's actually a good idea.
So that'd be a really nice way to build up your priorities.
Yeah, for sure.
Like automated linting and things like that.
We already talked about the bots providing the nitpicky stuff and so on.
You also suggested possibly having multiple reviewers.
Yeah.
Not a one-on-one situation.
Yes, I think a lot of people actually assume that core review is essentially one-to-one.
You have one person doing the authoring and one person doing the reviewing.
You can get real benefits from having multiple reviewers.
So this is something I hadn't done before working on OpenStack.
And in OpenStack, every change has to have at least two core reviewers.
And also then they'll often have other reviewers that are just community members that are interested.
So every change is, I actually looked at the numbers at one point, every change on average
was getting between three and five people reviewing it.
So they were a huge number, but only two of those were going to be in-depth reviews.
And it's just really surprising to me how often I will review something and then someone else
will come along and review and actually see something that I missed.
It helps multiply the benefits of the shared knowledge and the shared understanding.
It also takes the pressure off code review.
So you can not feel like you're the only person that's verifying this.
You don't feel as responsible, but you share that more.
It's not like you're the goalie.
And if it goes past you, it's over, right?
Yeah, exactly.
Interesting.
Yeah, definitely talked about the short reviews.
Do you think it would be beneficial to kind of reverse that as well and have multiple contributors?
So if I'm submitting a patch, this probably works better in a corporate space.
But if I'm submitting a patch and it's getting reviewed, I also have to like sit in on the review,
maybe be a reviewer, maybe just observe someone else's code being reviewed as well.
Like having that other side of the story not be one to many.
And that sounds a little bit like doing pair programming in maybe an asynchronous way.
So you have multiple people contributing to the project.
And I think that can work well, but it does probably require that you know people a bit more.
So this is something that I do see happen in OpenStack fairly often.
Some of my colleagues, if I see that they've got a review up and then there's a small addition or something else that I can make to it,
I'll just push a change to that.
I guess it also requires the tooling to support this.
It wouldn't be quite tricky in GitHub, for example, because you'd have to send a pull request to the branch,
which is the original pull request, and it becomes a message.
Yeah, yeah, yeah.
I'm certainly thinking like this is a place where you guys sit down over like a shared projector and have this conversation.
You know, something like that.
They get, you know, pure open source where it's all asynchronous.
I'm not sure it makes any sense.
I work from home remotely, so I always think about everything as being async and whatnot.
Yeah, yeah, same here.
I work from my house as well.
Okay, so all that stuff is really interesting.
Now, we talked a little bit on the tools that you might use to make this work.
So we've got Garrett, which is open source Python.
It's like a web app that you install.
Do you like connect it to your Git repo and it watches the branches or how does that work?
There are integrations for GitHub, which I've not actually used them very often.
But the way you interact with Garrett is with a Git plugin, which is actually also written in Python.
So you pip install a project called git-review, and that gives you a new command, which is git space review.
And there's a little configuration file in your code repository,
and then that will send the review up for review to the destination of the Garrett installation.
And you will have an account with Garrett already with your SSH key in there, so it knows who you are.
Nice. I see.
So you do like a push to that origin or something, right?
And then it has it.
It doesn't necessarily have to hook into the full code repository.
Yeah, so the git review, it does a couple of different things, the command.
But primarily what it does is it sets up the remote for you and pushes and creates a branch for your change on the remote one
and gives you like a review number and that kind of thing.
Yeah, yeah. Very cool.
And then GitHub, obviously, we've talked about that a lot.
They just added like full proper code review where you can go to a check-in and like expand it out and comment and stuff.
So GitHub's coming along.
There's also Bitbucket.
Probably a lot of people use Jira.
I don't know the source usage relative to GitHub, but it seems like at least issue and review stuff happens a lot in Jira.
I'm not sure if Jira has code review built in, but I've not really used it.
The one I do hear quite good things about is Fabricator.
So this is actually written in PHP, I think.
And I also think it came out of Facebook originally.
I've heard of some people really having success with that.
Again, I've never actually used it myself.
The only two that I really have recent and extended experience with are GitHub and Garrett.
Sure.
Okay.
Excellent.
So some tools out there, I know actually in this whole space, there's like a billion tools, right?
There's a whole bunch of small things, but these are some of the big players in our space.
Very cool.
I think, you know, we're kind of getting to the end of the show.
We're running low on time.
Let me wrap it up with my final two questions, as always.
So if you're going to write some Python code or any code, what editor do you open?
I use Vim all the time.
Yeah.
I work in so many remote servers and VMs that I have some scripts to kind of set up my Vim environment on that machine.
And then it's just, it's great.
Oh, nice.
Yeah.
That's super portable.
Awesome.
Okay.
And favorite or favorites from the PyPI package.
Have we passed 100,000?
I'm going to look at what you're thinking.
We're definitely getting up there.
It is at 99,706.
I think tomorrow will be the 100,000 packages.
So there are rounding up 100,000 packages on PyPI.
What ones do you recommend for people to check out that are awesome?
So I think most of my favorites are the fairly common ones that people know about.
So I picked a couple of more obscure ones here, which I think are just quite nice.
And they've been useful in my development process.
So Vulture is one that you actually covered on Python Bytes.
You referenced one of my blog posts about it.
And basically, it's a tool for finding dead code.
So it finds functions which are never used.
And that kind of thing.
It's just really useful.
I think that's a really cool project.
And I'm glad we talked about it.
Because nothing is more frustrating to me than spending half an hour trying to understand what a function does in the context of an app.
To realize, oh, I don't understand it because it doesn't do anything.
Right?
Like, it's super cool.
So very nice.
It's amazing how often I've found projects which have a function and it's only has a unit test for it.
But it isn't used anywhere else.
But because there's a unit test, everything that inspects the code thinks it's being used.
But with Vulture, you can exclude any uses in your test cases, for example.
And then it'll highlight it.
So that works quite nicely.
Oh, very cool.
Very cool.
All right.
What's the next one?
The next one that I've been using recently is called pip debt tree.
So that's one word.
And what you can do with pip debt tree is you can provide it the name of a package which is installed in your virtual land.
And I'll actually give you the tree of requirements for that.
So you can see where another package came from, why it's there.
Right.
You might be looking at your virtual environment via pip list and go,
why do I have beautiful soup installed?
Like I didn't install that.
Right.
And this would tell you, right?
It was installed because of this, which installed that, and it needed that or whatever.
Yeah.
And it prints it out in quite a nice sort of tree structure.
And then it's got a few other things.
Like you can do a reverse of that.
So you can have it tell you all the requirements for a package,
or you can have it trace where something came from, like the opposite end.
So from the leaf node of the tree rather than the trunk.
Oh, that sounds awesome.
I didn't know about that.
And the final one you said is entry point inspector.
Yeah.
So this is probably the smallest one.
And I guess this is because I do quite a lot of Python packagings.
But if you have, so say for example, when you install pip deptree,
it creates the pip deptree command for you that you can use in your terminal.
But actually trying to find out where these commands come from is quite difficult.
So entry point inspector will actually inspect the Python entry points that are defined in different setup.py.
And then I'll list them out.
So you can see where all these commands are coming from, where they're defined, what packages are coming from.
That sounds really cool too.
These are all great.
Awesome.
Thank you for sharing them.
Final call to action.
What should people do after listening to this?
They're all inspired about code review.
If people are inspired about code review, they should just think about how they can improve their process.
They should start thinking about what they're doing, build improvement into your code review process.
So every now and then maybe have some kind of retrospective and think about what's working, what isn't working, and just try and improve that.
Yeah, remember why you're doing it, right?
Yeah.
Absolutely.
Okay.
And then EuroPython's coming up, right?
Yeah.
So I did touch on this earlier, but EuroPython is going to be this year on July from the 9th to the 16th.
These dates are officially tentative, but I think they will be finalized by the time this episode is out.
I have to put that disclaimer in.
And people should start thinking about talks.
So we will have a call for papers coming out very soon as well.
I would love for people to speak.
If people have never spoken before, I'd be happy to speak to them about it.
and help them sort of find something that they want to talk about, I think that'd be good.
Yeah, it'd be great.
It's a very welcoming community, so a good place to get started with public speaking, right?
Yeah, absolutely.
It's where I first started speaking.
Well, Dougal, thank you so much for being on the podcast and sharing all these ideas with us.
It's been great.
Thanks.
I really enjoyed it.
You bet.
Bye.
This has been another episode of Talk Python to Me.
Today's guest has been Dougal Matthews, and this episode has been sponsored by Rollbar and Hired.
Thank you both for supporting the show.
Rollbar takes the pain out of errors.
They give you the context and insight you need to quickly locate and fix errors that might have gone unnoticed until your users complain, of course.
As Talk Python to Me listeners, track a ridiculous number of errors for free at rollbar.com slash Talk Python to Me.
Hired wants to help you find your next big thing.
Visit Hired.com slash Talk Python to Me to get five or more offers with salary and equity presented right up front
and a special listener signing bonus of $2,000.
Are you or a colleague trying to learn Python?
Have you tried books and videos that just left you bored by covering topics point by point?
Well, check out my online course, Python Jumpstart, by building 10 apps at talkpython.fm/course
to experience a more engaging way to learn Python.
And if you're looking for something a little more advanced, try my WritePythonic code course at talkpython.fm/Pythonic.
Be sure to subscribe to the show.
Open your favorite podcatcher and search for Python.
We should be right at the top.
You can also find the iTunes feed at /itunes, Google Play feed at /play, and direct RSS feed at /rss on talkpython.fm.
Our theme music is Developers, Developers, Developers by Corey Smith, who goes by Smix.
Corey just recently started selling his tracks on iTunes, so I recommend you check it out at talkpython.fm/music.
You can browse his tracks he has for sale on iTunes and listen to the full-length version of the theme song.
This is your host, Michael Kennedy.
Thanks so much for listening.
I really appreciate it.
Smix, let's get out of here.
Outro Music.
you
.
you