Pull Requests with Joe LeBlanc

Joe joins the panelists to talk about pull request etiquette.

Joe Leblanc: Joe first learned to code on a Zenith computer his dad brought home from work. It had this built in blue LCD monitor and ran on 5 1/4" floppy disks. He used spreadsheets for work and Joe was interested. They spent about an hour going over macros together and he took off from there. Long after the Zenith died, the open-source content management system Joomla! landed in the center of his attention. Joe found himself writing a book about Joomla programming, authoring video tutorials about Joomla for lynda.com, giving Joomla talks, and helping organize Joomla conferences.

Since his time in the Joomla community, he's picked up Node, Rails, React, and other frameworks. He's currently coding at True Link Financial and working on a few hobby-projects as well.

---

Please join us in these conversations! If you or someone you know would be a perfect guest, please get in touch with us at contact@frontside.io. Our goal is to get people thinking on the platform level which includes tooling, internalization, state management, routing, upgrade, and the data layer.

This show was produced by Mandy Moore, aka @therubyrep of DevReps, LLC.

Transcript:

CHARLES: Hello and welcome to The Frontside Podcast, the place where we talk about user interfaces and everything that you need to know to build them right. My name is Charles Lowell, a developer here at The Frontside. And with me also is Taras. Hello, Taras.

TARAS: Hello, hello.

CHARLES: Today, we're going to be continuing our conversation about platforms, as always, but in particular the pillar of your platform that has to do with how you collaborate on code. It's an important one. And so, we're spending some time on it. And with us to talk about this today is Joe LeBlanc who is a senior software engineer at True Link Financial in our very own beloved Austin, Texas. Hey, Joe.

JOE: Hi.

CHARLES: Thanks for coming on the show. We're going to be talking about collaboration and I was thinking we could kick off the discussion talking about pull requests because that's typically one of the ways that we collaborate on code. That's really where the rubber tends to hit the road. You have particular interest in the dynamics of a pull request. What experience kind of led you to that interest?

JOE: My background has been doing a lot of work as a freelancer. And when you work as a freelancer or do agency work, a lot of times you are really the only software developer that is around or you're working with maybe one or two other people. And I didn't get a lot of opportunity for pull request reviews. Mine was in that space. And then when I moved to a full time job at a place where I was one among maybe half a dozen or a dozen engineers, that's when I really began to get interested in how I could be better at giving pull request reviews and also submitting pull requests that people want to review.

CHARLES: What made you notice the need for this?

JOE: What would happen is someone would submit a pull request and there would suddenly be just dozens and dozens of comments coming in that were just kind of difficult to keep track of and often were maybe talking about things that didn't necessarily need to be reviewed as a part of that pull request or addressed, maybe things that could be caught by a linter.

CHARLES: Right.

JOE: And other tools that are a little bit easier to receive feedback from. Like it's easier to have a tool tell you your spacing is off than have me tell that to you.

CHARLES: Right. And really that seems like the spacing is off, that's kind of like if you need to deliver that feedback, that's kind of like not what you want to lead with. If you don't have linting in place like after all other issues are sorted out.

JOE: Yeah.

CHARLES: But it sounds like what you're describing is people who have kind of swarmed over a pull request each with their own kind of pet peeve issue.

JOE: Yeah. And then you're just left with this long list of comments to go back and address and you're pushing up more and more commits. And by the end of it, you could have more than 100 comments on this PR. And you thought that you were going to get this done in a day or two, and then suddenly it's the end of the sprint and it's like, "Oh fine, then I get to merge this."

CHARLES: And typically, it's actually, in my opinion, an anti-pattern when you have like 10 mergers that happen on the second last day of the sprint or the last day of the sprint. There's always issues with that, right? Ideally, you are merging code throughout the course of the sprint. It kind of like defeats the purpose almost. I guess you're doing your integration in shorter periods but even so, tons of stuff is bound to break when everybody's pushing and everyone's rushing.

JOE: Yup.

TARAS: So, what would happen in that situation, in your experience, when you have this pull request that a lot of people are commenting on and some of the comments could be addressed by linters. Would you guys go back and actually implement linting? Did that happen? Would you put linting in place so that you'd have consistent formatting on the projects to reduce that kind of feedback? What would be the resolution or a way to improve on a process so you could actually get better feedback?

JOE: Yes, we did install a linter and that's something that you can either run locally or you can also run in your continuous integration environment. And it's really good to run it locally first because then you can just catch it before you even submit the PR. And that was something that definitely helped cut down on these sorts of comments and even debates over style. You just have this one thing that is maintaining that style or rather telling you when you're wrong for you, and then you don't have to have all these comments coming in as you submit your PR.

And another thing that we would do is we would use, even beyond just things like whitespace and trailing newlines and those sorts of issues, sometimes you would have some of these issues that would come in that wouldn't necessarily be a trailing newline or whitespace issue or something like that but it would be purely someone's sense of style. And in those situations, we came up with a bit of a standard where if you wanted to make a comment that was strictly style that wasn't something that you necessarily wanted to block the pull request over, we would use emojis that had specific [inaudible] thing. So in our case, we would use either a beer emoji or the cocktail emoji to kind of say, "Here, take this with a drink."

CHARLES: [Laughs]

JOE: This is not something that's super serious. It's a suggestion, "Here, let's discuss this over beers."

[Laughter]

TARAS: It's a really nice approach to friendlify the actual pull request because people can get really -- I think another way people would say this is like when you say that it's not 'I believe it whole strongly'. How does that go, Charles?

CHARLES: It's strong opinions, weakly held.

JOE: Yes.

CHARLES: Right. You're kind of making an assertion about the way things ought to be or often missing from that is how important it is to you. You've made this thing like it should be like this. That's a very black and white statement, very firmly cut between right and wrong. But how important it is to you if it goes the other way is often missing from the conversation.

TARAS: So Joe, what would happen once you made this change? Did you see a shift in the kind of feedback you were getting? What did it look like after this?

JOE: After we started implementing the ideas of putting on emojis to signify things that were not super serious, it became a lot easier to just picture all those comments and define the things that really needed to address. And it was a lot faster. I was just able to address these very specific problems and do that first. And then if I had time later, come back and address those style issues.

CHARLES: Right. Was the kind of the good feedback there the whole time but it was just obscured by the noise of these kind of exterior issues or side issues? Or did you find that because people's thoughts were more given over to the heart of the matter, that you actually got more comments directly relating to kind of the meat of the implementation?

JOE: I think it was really more that the good comments really were buried in there. A lot of times when you see critical comments that seem to be petty, that can really trigger your emotions. And so, I feel like a lot of the times the emotional state that we would get into would prevent us from seeing the good comments that people were leaving, and we would get a little too angry about things that really weren't that big of a deal.

CHARLES: Would the tension kind of noticeably rise?

JOE: Yeah. I actually remember several instances at my job where -- this is when I was working in office and in the same room as all my co-workers. And one of my co-workers would know that I was reading his review by the number of sighs that were coming out of me.

[Laughter]

JOE: Yes, I definitely had this reaction that people could tell. Implementing things like these emojis definitely helped. They didn't always fix everything, though. One thing that would happen too is even after filtering out kind of the noise comments and getting into the meat of the matter, we would still wind up in situations where I would want to use one design pattern and another engineer would come along and say, "No, you should do something different," and we would have these arguments.

CHARLES: Right. And those are harder to resolve too because those are strong opinions strongly held.

JOE: Yes.

CHARLES: But it's still a step forward, right? You've eliminated the passionate arguments about the strong opinions weakly held. And so you're able to now grapple with 'how do you resolve these arguments' of these stronger opinions that are more strongly held.

JOE: Yeah. Part of the way that we handled this was we came up with a way of being able to have a debate about something on a pull request, yet also keep in mind that we're working under deadlines and that ultimately, it might not be that big of a deal.

TARAS: I don't know if you experienced this but there's this kind of a feeling where you know you have a deadline that you want to reach and this feedback is kind of not really helpful to that end goal. But at the same time, there feels like a trade off for you, like trading off quality by accepting something that people don't agree being fully ready.

JOE: Yeah.

TARAS: But then there is, I guess the tests seem to be like you have some tests and the tests are passing. So, thumbs up. [Laughs]

CHARLES: Yeah. Well, here's the thing. This thought just occurred to me when I heard that is when we are reviewing code, probably the most important code to review is actually not the implementation but it's the tests. Because why do people get so care mad about the code being right? It's because they know how expensive it is to get it wrong. And we all have our different opinions of what's right and wrong and what's going to cost us more. But at the end of the day, we're all trying to save us, save ourselves and our team, pain in the future. The tool that we have to do that is the code review. We're trying to make sure the code is "good". But usually what ends up happening, and I know I'm certainly guilty of this, is I'm focusing on the implementation and I look in and say, "Oh yes, there are tests." I don't really look at the test and say, "Is this a good test?" Because a good test is going to lower that cost that we're so afraid of in the future, and that drives us to want to make sure the code is as close to what we conceive of as perfect as it's likely to get and still hit our deadline. It sounds to me like maybe what we really ought to be doing is reviewing the quality of the test because if you have an excellent test, then the shape of the code almost is unimportant or changing the shape of the code, more importantly, is very cheap. So if you do find that the implementation is suboptimal, the cost of rejiggering it into a better configuration is going to be very low.

JOE: I've discovered so many times where you are running a test that was written previously either by somebody else or by yourself, and you realize that the test is testing the wrong thing entirely.

CHARLES: Yeah, coverage is spotty. It covers 10% of your cases.

JOE: It can also might have a stub or a mock in there somewhere that is hiding something that you weren't counting on. Or it could just be you pull up this screen and test then it saves but you're not really testing what it's saving.

CHARLES: Right. Can I actually proffer a suggestion? At least I'm going to try and hold myself to going forward, is that when I review a pull request or some proposed change to a code base, I'm going to review the test first.

JOE: Ooh, that's a great idea.

CHARLES: I'm going to try and start with the test and avert my eyes from the implementation, no matter how curious I am how this person accomplished the solution. I'm going to hide from the solution and focus on the verification. And if I have faith, first and foremost, in the verification, then my faith in the implementation is secondary.

TARAS: You know, Charles, I really like this. I think it'd be really interesting to try this out and see what kind of impact it creates. It makes me think that there is actually something really useful in here in terms of providing feedback because the challenge is that when you're giving feedback to someone on a pull request, ideally that feedback is going to be useful and it's going to give the person something to think about in regards to the work that they did. And I think a good way of pushing to work back onto the person in a way that allows them space to internalize what they need to do is identifying cases that might be missing because a lot of times, we test happy paths. We don't test the things that we didn't think about. And that's where the devil is in details. It's in the things that we don't test. Those are the kind of problems that we're trying to prevent with having good code. But really the best way to kind of flush those things out is to make sure we have the right test coverage for it. So yeah, I think if you add to that just when you're looking at a pull request is to go, is a test coverage missing the following cases and actually surfacing not. I'm actually thinking like a danger task. A danger is a tool for adding information to pull requests so that it's kind of like an automated mechanism to comment on pull requests. And so I'm thinking like having something that does what [JAS] does. [JAS] has this thing where they will check based on the last git commit. It will figure out what are the tests that have been added since the last commit and then it will actually show you. So when you run tests, it only runs the tests that you kind of impacted which is something that could be used to surface tests that were written in this commit. I mean, we also have that information in the actual commit itself but being able to see 'these are the tests that were written or added' and then you could use that and figure out what else is missing from this. This could be like a way to kind of add onto this process, something a little bit more automated so you could actually highlight this information very easily in the commit itself.

CHARLES: It's funny, my mind was wandering off towards GitHub. When do you use danger and when do you use GitHub actions? GitHub actions have been kind of like brewing and fermenting in my mind.

TARAS: One nice thing is that I mean, danger is a little awkward in that it creates one single commit at a topic. It creates a comment first time you commit and then it keeps pushing information into a comment which shows up at the top of your comment thread.

CHARLES: Right.

TARAS: But that's not how you read comments. You read like top to bottom. You don't read top to bottom to top. And so, I think the nice thing about the actions is that because it's using checks like GitHub checks that actually is part of a different area, it's part of the validation area as opposed to part of the actual comment conversation. This seems like a more appropriate place to put their information rather than the first comment.

CHARLES: Yeah. So, do we decide on kind of what the ultimate resolution is for when you have these high order conflicts?

JOE: Yeah. So that's one thing that we came up with that I'm really proud of. And so what we do and at least at that job that I was at, what we did in those situations was we said, "OK, if you've gone back and forth on this two or three times and there's still not an agreement as to what should be done or there is no compromise, what you should do is let the author of the pull request go ahead and merge that code," because you want to merge that code, get it out to production, make sure it's serving our users and our clients. And then what you want to do is take that conflict and record it somewhere to specify this interpersonal conflict not a code conflict or a merge conflict or anything like that. So you take this interpersonal conflict, the topic that was being discussed and you put that -- we were just putting it in a Google doc and leaving it there. And then every couple of weeks or maybe every couple of sprints, we would go through that Google doc and just talk about topics that had come up. And then really more often than not, what we would find is that we didn't care about the topic anymore.

CHARLES: [Laughs] It's amazing how time has a way of cooling passion over things that really don't matter.

JOE: Yeah. And for the things that did matter, we usually have like a really good discussion. Sometimes, we even came up with something different entirely based on just having more people in the conversation and thinking about this problem.

TARAS: I like this because there's a bit of process around it. It's kind of like a retrospective on pull request passions.

JOE: Yes.

TARAS: It sounds like a healthy thing to do for a team, especially. And I think just allocating some breathing room to go through these kind of things could be really important cumulatively over time, especially.

CHARLES: Yeah, definitely. It sounds almost like the pull request is then ongoing. You have this collaboration that happens kind of at the front of the change but then is rippling outwards and onwards and hopefully then has an impact on future changes, like if you're disagreeing. So, would you qualify that the types of issues that end up living in this Google document as architectural issues or just, 'hey this is the way we need to talk to each other and this was off and we need to fix this' or is it both?

JOE: It was mainly more architectural decisions that was coming up in this, sometimes like code style issues as well. Not so much the interpersonal issues, like the interpersonal issues would come during retros.

CHARLES: OK, because it sounds to me like it's not. Then you have the added benefit that your architecture does get to improve because clearly if there's some disagreement about this, then there's some sort of tension there. There's someone perceiving that some problem is not being resolved by a particular implementation. And so it's good to just at least surface this issue again and again. What was your experience with kind of revisiting the architectural issues? Was it mostly 'well, this wasn't really an issue' or is it 'wow, I'm really glad we took note of this because now we have these three ways of thinking about things' and it turns out that given three or four months more experience, this is something that we should be doing.

JOE: Yeah, it is definitely a mix of both but I'm kind of leaning more towards the latter. Like we're just glad that we bring things up. Occasionally, there will be one or two topics that will come up that we still would resolve and we would have to agree to disagree on something. Or in some cases, I think we would allow one of the lead engineers to just make the final decision on something. But that was pretty rare.

CHARLES: So, apart from appealing to either time, the passage of time or just kind of taking a brain dump of all the different architectural options or deferring to a more senior engineer and just kind of asking them to step in and make the call, are there any processes that you can put in place say to kind of mechanically come up with a decision? Like any set of values that you can use as a ruler to kind of line up a set of things to kind of attach weight to one particular solution? For example, one of the things that I always think of is when I have internal conflict about a decision, there's kind of a part of me that feels like this might be the right way and maybe another way is the right way. And so, a tool that I will use is what's internally consistent with the rest of the system. So, even if I've had an insight that something really ought to be framed,one solution should be framed in a certain way. If there's another solution that's more internally consistent with the way things are existing, then I'll use that as a discriminator to say, "OK, that's one thing that I can use to measure a solution against, that I'm not emotionally tied to." It feels more objective. And if you have those tools, you can kind of pile them up and see which pile ends up being higher for each solution. Do you see what I mean?

JOE: I definitely lean more towards keeping things consistent with what's already there. Personally, I probably do that to a fault. Sometimes I need to branch out a little more and get comfortable with possibly breaking something. But you always need to weigh whether it's something that is acceptable to break. Like a lot of the systems I've worked with involve money and it's really important that people either have money when they need it or are charged the correct amount when you charge them with things.

CHARLES: They get really mad when you charge them too much. [Laughs]

JOE: Isn't it amazing? [Laughs]

CHARLES: Yeah.

JOE: It could be that you are OK with breaking this other end of it where you might charge them the correct amount but the fulfillment of that product or the service or whatever you're selling, you might have a little more wiggle room in that part of the code base. Where if you break something, somebody calls in and says it's broken, and then you're just able to fix it.

CHARLES: Is there anything else? If folks are struggling with the way their pull requests work or their code reviews work and they're experiencing friction and tension with their teammates, is there anything else they can do?

JOE: What I would suggest is definitely look at the pull request template feature that is in GitHub and make use of that. Because what you can do with that is come up with several sections of the pull request template and say, "These are the questions that we want to ask before we submit a PR." And if you have those questions and those sections right when you go on that template and people read through that as they're making their pull request, they can often catch problems before they get to another reviewer. I can't tell you how many times I have submitted a pull request, started to write the description and [inaudible] these questions and realized, "Oh, this is wrong," or it's broken or it's totally the wrong thing.

CHARLES: Right. It's funny and I feel like you want to be asking those questions not when you're submitting the pull request but before you're even writing the code.

JOE: Yes.

CHARLES: But a lot of times, we don't do that until afterwards and you're like, "Oh man!" Just this process of forcing myself to think about the problem in this way or think about it holistically totally recasts my implementation.

JOE: Yeah, things like, "How can this break after we merge it?"

[Laughter]

CHARLES: Yeah, that's actually a great question, that of one you're talking on pull request.

JOE: Oh yes, definitely.

CHARLES: Like imagining the [inaudible] scenarios.

JOE: Yep.

CHARLES: Man, it's almost like you want to read the -- what are the other questions that you have?

JOE: So the ones that we have are, first of all, what is this pull request? How does it fulfill the requirements for the ticket? And then how can this break after we merge it? Are there any post-merge tasks that need to be run? If you need to get on a server and run a task or do something after it has gone to production, that's a place to document that there. And then the final question is, how is this tested?

CHARLES: Right. That's a good one. And like I said, I think that's the one where I'm going to be starting when I'm both thinking about how my changes will be reviewed and how I review changes.

JOE: Maybe I should consider moving that to the top position.

CHARLES: [Laughs] I mean, it is like. I'm just thinking about it and it does seem like we go straight to the implementation because that's what's fascinating to us. And so we might have way too much bias for the importance of the implementation over the importance of the test because I have to say I am so guilty of when I'm reviewing. Just looking for the fact that it has a test and not looking for what the test does.

JOE: Yeah.

CHARLES: And only referring back to the test and trying to understand how the test accomplishes its task. If I don't understand the implementation, I'm like, "Oh, let me look at the test and see how this code is used to the test." That might be problematic but I know that's definitely a little personal goal that's coming out of this podcast for me.

TARAS: Excellent.

CHARLES: All right. Joe, do you have anything else that's coming up? Any talks? Any meet-ups that you're going to? Any announcements? Anything that you want to plug?

JOE: I have a website at JLLeBlanc.com and that's where I am.

CHARLES: All right. Well, fantastic. Thank you so much for coming on.

JOE: All right. Thank you.

CHARLES: Thank you for listening. If you or someone you know has something to say about building user interfaces that simply must be heard, please get in touch with us. We can be found on Twitter at @TheFrontside or over just plain old email at Contact@Frontside.io. Thanks and see you next time.