 This is the Tiffany room, and you are about to see code review forwards and back by me Sumana Harihareshwara, and by me, Jason Owen. All right. Let's get started. Hey. Hey. So I just saw your message. Yeah. It's now still a good time for the code review. Yeah. If it's good for you. Is your feature branch up to date? Yes. I just pushed all my latest changes, and it's ready for you to take a look. Okay. I am pulling it down now, and most of the time, we are not going to need to do this face-to-face, right? We'll do it online on GitHub, and remind me, okay, what is this for? Okay. So this is for bug 1104, which is to stop parsing the HTML and start using the API. Okay. Yeah, I remember this now. And I figured while I was working on that, I could clean up the code a little bit, add a bug class to take care of some of these interactions, and, of course, add a test. Okay. So just to start with, you know, having your commit messages say that with an active verb as the first word is, also, this commit message at the start, you're going over 50 characters in the first line, and we really don't like to do that. So the reason this is messy, you're not taking notes? Oh, sure. Do you have a... Yeah, there's a pen there. Okay. So here in test underscore extract underscore bug underscore info, you see how you don't have spaces after the commas in the parameter... That makes it harder to read. Okay. What? What is this? What? Okay. I am not really used to caring about these kinds of things. Like in my assignments, if the code works, then it passes, and if it doesn't, then it doesn't. So like... That's... Yeah. I mean, working is important. That is definitely the most important thing. Okay. So I guess by focusing on this, you're saying that these things are important. Yes. Okay. Look. This is small, but it's about consistency. If all of the code in the code base is consistent and looks the same, then it's easier for us to change it, and because it's easy to see what's different in every diff. Right? We're not adding or changing white space or commas. The code is consistent. If all the code in the code base looks the same on a stylistic level, then when we review diffs, it's easier to actually see what changes. What's really important underneath of the logic of the piece. Okay. Right? It matters. Here's another. So you've added Retriable and HT Party as gems to this project. You've put them at the end of the gem file. Yes. If you keep a gem file alphabetical, then it's easy for someone five months from now to check and see whether a particular gem is used in the project. Okay. I mean, I can fix that, but couldn't they just use search? Your colleagues are capable of using grep. But with every commit you make, if you think about developer experience, right, think about how can you make it easy for someone else to read this code if it's two in the morning, they rolled out of bed because they got paged and then they are, you know, hit and scroll to see what it does, then if you think about developer experience in every commit, then you're making it easier for possibly you in the future to know what your code does. Okay. All right. So you know what to fix? Yeah. Okay. So you have a gem file and you fix the commit messages. Right. Okay. Here, let me show you the developer experience, the first line of a commit message, if it's too long, it gets truncated. Right. I noticed that on GitHub too. Right. And then when someone's running Git log, that's also, so it's annoying. Okay. So, okay, to back up a little bit, the point of this issue is if we're integrating, you know, a CMS and a bug tracker, the feeds from it, oh, someone's pinging me. Oh. Okay. Okay. So I'll fix those things. Yep. Yeah. Fix, check the style guide, look at some other code, make sure it matches up what's in the existing code base and then it's, you know, it's probably good, so go ahead and merge it. Into master? Mm-hmm. Okay. Oh boy. Okay. So it's been two days and Randall is wondering why the site went down. Oh boy. This is going to be hard to explain. Why does he, is this for the blog posts? Oh, no, no, no, no, no. There are only three kinds of corporate blog posts. At least get your friends to try our new beta. There's one we've gotten very good at, which is we apologize for the recent outage. And then there's one we may be doing soon, which is thank you for our incredible journey. Okay. So this is about explaining to the hairups tomorrow. Yeah. Oh yeah. There is going to be a meeting tomorrow in a big room where I explained to a number of people with interesting titles why we had this outage and why, why we didn't prevent it really. So on the code level, we figured this out, right? Like we were streaming the data into the disk cache and that worked when we had the VM get it replaced every deploy. Right. And then we were deploying every couple days, but Darren went out of town for a week or two and so we didn't have any deploys in that time. That is the most robust thing I've ever heard. And so the disk filled up and the application crashed. They are going to ask the extremely reasonable question of why we were not instrumenting and monitoring this extremely important process. We thought we were. We wrote code specifically to handle the situation, but it turns out that that code just never ran. How did we not notice that? I mean, there's another one that's, another method that's named similarly and we used the wrong one and it doesn't do the same thing. And no one saw. Who would have seen? I am going to have to explain why we switched to self-review. You think we would have caught this in code review? Yes. We never caught stuff like that. We were always focused on like the silly RuboCop level stuff. Our code is super pretty. It just does useless things. You know, if I could do a few things over again. Oh, hey, yeah, I just saw your IRC message. Yes. Is now still a good time to double check the code review? Yeah. Yeah. I think that both you and Randall had said some things on it, so I wanted to check in with you. Oh, okay. I'm pulling it down now. The feature branch is up to date. Yes. I just updated it based on some of Randall's comments. Okay. Remind me what this does. Yeah. So this is for bug 1104, which is to stop parsing HTML and start using the API. Okay. I remember now. Yeah. And I figured while I was in there I could clean up some of the code, add a bug class to handle some of these interactions and of course add some tests. Okay. That's... Okay. So why did you put the bug class in this file? I mean, it made sense to me there. Oh, that should go in our utilities library. Okay. So one of the comments that Randall had on something else was that classes should be close to their uses. Randall is very often right about a lot of these kinds of things. Okay. So across the organization we are trying to, in order to make the developer experience more robust, we are trying to consolidate interfaces to external resources. And so that's why that should... I mean, it's more discoverable that way. And so that's why the utilities library is the house... That's where you should put that. Okay. Yeah. So that's where you should put that. This test, the name of it, you should rename it to be consistent with the rest of the... With snake case, with underscores instead of camel case. Oh, yep. Sorry, I got that. Okay. All right. So here you're using HTTP client as the base case. That's actually deprecated and you'll need to use API client. Do you know where that is? I can find it. Well, yeah. I thought the HTTP client was the new thing. Who told you that? Randall did. I have a lot of comments on the pull request. I originally wrote it and just switched. So there's a number of component choices, including this one where... I'm sorry that it didn't come up yet before the architecture committee. There's this sort of... The cross org taskforce, I remembered it being on the previous agenda. Now I remember it got pushed. So I'm sorry. I thought we'd already made a decision on this one because... Okay. But anyway, it's for the meeting next week. So we need to actually check which one we're actually going to use across the org. It's kind of a formality, but we need to do a bake off. Oh, really? Yeah. I mean, look, API client is clearly technically superior, but in fact, I can use your branch as an example. Did you just switch it? Okay. So anyway, I can't merge all of this pull request until that's finished. Okay. When will that be? So let's see. Another week that they meet, and the bake off starts next week, and then, oh, right, Thanksgiving and Mike's out. So five weeks. Five weeks? Okay. Look, I know how this... We want to be robust and defensible in our component choices across the organization so that team members who need to switch into other projects and reusing the same components across the organization. In the long run, you're on the same tool chain that's going to increase organizational velocity. And so, look, okay, if you change the bug class, move that, the utilities library and clean up the test name and so on, I mean, I'll let you know what the architecture committee decides. Maybe we can speed it. You have runway, right? You have something else to do? Yeah. I can figure something out. Okay. Great. Yeah. Oh, so I was hoping that we would have your help with the rewrite, but congrats on the new gig. Thank you. I am super excited for this opportunity and it just felt like the right time to move on, you know? Right. Well, okay. So it's two weeks, right? Mm-hmm. Okay. So let's see what we have you working on for your last two weeks. You were figuring out the Sinatra to Rails migration number 210. Yes. That PR got a lot of comments, so it's going to take at least a few rounds to address all of those. More than two weeks? Definitely. Okay. What about you were also working on bug 1509 over here, right? Yes. So I got that working on my machine, but Sam pointed out that it's not going to work on Oracle and DevOps can't give me a new instance until they get the new hardware, so I'm blocked on that. Oh. Okay. What about 1677, the security one? Yeah. That one I'm blocked on the security team. Yes. They don't have any capacity of this print. You asked them? Yeah. I'm not going to get this out, but they have other stories that are higher priority, so. Okay. So how many bugs do you have assigned in JIRA? Like 12, I think. Okay. Well, I mean, most of them we can close. I'm going to, you know, because we can won't fix them because the rewrite is going to obviate them. Oh, sure. Oh, yeah. And that's pretty soon. So I can do that. And then, okay, on Sinatra to Rails, start addressing just as many of those, like at mention me on all as many of those comments as you need to and assign, or no, I mean assign stuff to me, but first see what you can do this week and I'll hand stuff up. And so then next week there's a new dev starting and I figure you can train them up and help them get to up to speed on the current system. Sure. And I mean, you could even like when you're on your way out, you can give them your dev box. Yeah. I'll get started on that tomorrow. See you. A few things over. Did I get a text message from you? Yeah. I was wondering if you had a chance to take a look at the code review. For you? For the, I mean, is this the, a feature branch that's, I mean, it's up to date now? Yes. I pushed all my layers changes. I was ready for you to look at. Now? Mm-hmm. Oh, okay. Okay. I'll put, okay, it's on GitHub under your, okay, yeah, I'm pulling it down. So I wasn't sure if I should like, prepare something for this meeting. What? Like a, I don't know, like an outline or a walkthrough or a side deck. What? For, for what? For the code review. Oh, oh, no, I mean, oh, okay. So like, we're, you're on ramping, right? So like, you're not going to have too many of these. Like once you finish on ramping, you're probably going to only ask for code review for really major changes. Okay. So, you know, it's only if it's big, right? But this isn't, oh, but actually you do have kind of a big commit here. Do you, that's a lot of lines. Actually, do you know how to use get add dash P when you're making a commit? I don't know. To make real tight little. No, I don't know. Oh, cool. I saw like a blog post about it. It was really great. I can text you a link unless you want, do you have any email or, or Slack it to you? Yeah, whatever. It's good. Okay. I don't know how much we want to be using this, the, the API that I don't, I don't know. I mean, if you're good with that, I mean, is it, is it getting deprecated or what's the concern? Not that I know of. Okay. I mean, I like places like the same time as the like rate limit or something, right? So I noticed that there was another class that was calling the API, the CMS helper, which is kind of similar to what we're doing, but was kind of distinct also. I kind of wanted to get your opinion on like whether that was worth trying to merge or if we should try to separate. Oh, we've actually got like, yeah. I mean, you could do that. Yeah. I mean, you know, you got to check a different behavior or what, like if you want to do that. Okay. So should I add some code to check for the rate limiting case? Does that's something you think you need to do? I mean, I mean, also, like, have we actually had that come up? We're at like 100, 200 requests a day, right? So I mean, if it doesn't come, that's just like extra code and it sounds like you want to like keep things like refactored and streamlined. Okay. Yeah. So, okay. All right. So that's what I saw. If you want to take a second look at that stuff, okay, so I will take a pass at fixing those things and then ping you to take a second look and merge it. Do you not have the merge proof? Like are you able to push to the repo but not master? Do you need to have me ask IT? I don't know. I mean, I haven't tried. I thought I needed a plus one before I could merge it. Oh, no, you could have merged this now. Oh, really? Like, yeah. I mean, like you asked for a code review. So you know, I told you a few things you could look at if you want and then you know, see what you think is changing, what you're pretty fine, you know, what you think is good and like, oh, okay. Yeah. Someone's going to micromanage you or I wouldn't have a heavy pre-commit review process like that. Is that like your last place? Kind of. Okay. So I'll just merge it. Yeah. Yeah. I mean, look, you have to commit bit because you got it when we hired you and we trust you. Great. Great. So I am just completely stuck. I have no idea what to work on next. Oh, are you done with 11054? I mean, kind of like I figured out what was going wrong. It was that Randall was subclassing from issue and Ray was subclassing from bug and this, like they're similar, but not the same. And so that different behavior, like it's not even obvious to me which one that we want. You're going to merge them together? I mean, I could, but like, which is correct. I can't ask PM because they're out on like a retreat right now. And their entire calendar next week is book full of readings. And like, what if functionality depends on. Right. Exactly. Yeah. All right. I mean, you're good at this kind of detective work though, right? Like your detective, Jason. Well, thank you. But like since Nancy left, all of this work has been falling to me and I can't do all of it. Like, there's just so much of this kind of problem that we're facing. Our code is a maze of twisty little functions all alike. You know, if we could do a lot of this over again. Oh, hey, Jason. How are you? Good. How are you? I'm okay. I think, did I see a message from you on Slack? Yeah. It's not so good time to do a code review. Oh, I can do that now. Yeah. Is your feature branch up to date? I mean, so, you know how Robin is working on continuous delivery? Yeah. So, my PR was the only one that was open. So they did a quick review and merged it to test. But it's ready for you to take a look at. So it's in master? Mm-hmm. Now? I see. What does this do? Right. So, this is for bug 1104, which is to stop parsing the HTML and start using the API. I remembered. And I figured while we were in there. I could clean up some of the code, add a bug class to handle some of these interactions. But, hold on. Jason? Mm-hmm. What's this? That's the API class, client. You wrote this from scratch? Mm-hmm. Wait. Is the date right on this that you wrote this, like, this is you who, last week did you start on this? Yeah. That's when Ray assigned me to this. Okay. I don't know why you didn't ping me to check this. I mean, we had a conversation. No. Okay. There's a few problems. A, this is redundant because we have an API client that we're using internal in this library. Over here. You should have pulled this. And Brian's. I didn't know about that. Brian's team wrote it. So, this brings us to B. This is not just one API for our bug tracker. This is related to our services platform that this API token connects to. That's for the shop, CRM, analytics, basically everything, which is why it makes sense to write a client that we used throughout the code base, what Brian's team has been working on. And that's a kind of finicky one. So, we need to be kind of careful about error handling. And I don't think you even looked at the docs when I see you. Yeah. It's basically zero error checking. You're not even checking for the status on this call, which always returns 200. What? Even if you give it bad data. I mean, you wasted, I mean, you spent a week on this. Yeah. Oh, you should really have pinged me. I did. We had a whole long conversation in Slack in the channel. No, no, no. You and other people may have had that conversation. But you didn't at mention and ping me. I can't keep track of everything in Slack. You need to use Brian's library. I mean, I know it's an okay. This is a master, but it's not being used yet. Right. Cause you hit it behind a feature flag. No one said anything about having a feature. Whoa. Okay. So we need to get this reverted yesterday. Cause if we're suddenly issuing a ton of these erroneous, which we may well be, thanks to your chat, they're going to cut off our API token and our account on our. What was it? You don't know how to use get revert. Do you? I can read them. Watch me. Uh, all right. Um, okay. The, um, the last sprint, I think went well. Uh, we're only carrying over three stories from the last one. Um, and, uh, most of those are part of the conversion from monolith to microservices. So, you know, we're going to break out and get real free and sales, uh, you know, towards our, uh, our stories for the next sprint. Uh, we have a few, um, a new, a new story, a new user story from sales from sales. Yeah. Do we have a customer? Not a current signed up ink on paper is dry customer as such, but, uh, sales is working on a new user persona where this story is supposed to be real, real helpful for something that they're, they're working on. Yep. So is this going to help us launch? Do we, do we know when that will be? We know it will be next year. We don't have a quarter set, um, for launch, but, uh, you know, we're, uh, we're on our way. Yep. So let's go ahead and estimate out these, uh, you know, on the board. Yeah. So now that I look at the new story from sales, um, why do they think it will help? I recognize that, um, you know, what we've been working on and, um, what they, it's not a hundred percent one to one integration with the, as though the communication has been, uh, with a, it's not as much of it on the face of it match, uh, with the core product that would all be together. If I could do a thing over again. Hey, thanks for coming over, Jason. Yeah. Okay. So I have some comments on your pull request and I wanted to walk through them in person because it's your first time. I do have a few little criticism. Those can kind of read harsh, you know, if you're like just reading them on paper, you know, online. So, and I, I didn't want you to take it the wrong way. Right. Yeah. Okay. So first off, this seems pretty good overall. Thank you. Right. But I am a little bit confused. It looks like, uh, you wrote your own tool instead of using an existing one. Could you talk a little bit about why you made that choice? Oh, um, I didn't realize one existed. I looked for it on Ruby gems, but I didn't see. Oh no, it's not a source. No, it's internal. Brian, uh, wrote it. It's a, he's in the Chicago office. I don't know if you know him yet. No, I don't. Um, so how should I have found that? Oh, oh, that is a good question. Um, I'm sorry. I just knew about it because a few years ago we started. And so I just looked in my email. There was a past email thread about it. And so I'll just find you that link and I'll send that to you. I think that's what you're going to want to use. Okay. Uh, that would have been good to know like a couple days ago. It's frustrating to have to throw out all this code already. Oh, I'm sorry. Sometimes that's just how it is. Um, I, I've been silver lining now that you've written your own. Uh, maybe you will find something useful in seeing how he solved this problem and, uh, you know, appreciate that. Heck, maybe you'll even be able to give him some ideas for improving it. Okay. Um, how would I make those suggestions? Oh, an email is fine. Okay. Um, you could ping him on chat. So okay, that's really the only big change that I see in this. Um, I have a couple of code style suggestions, but if this is more of a first pass work in progress, I don't want to like go over it. I mean, now it's good. Might as well tell me that so I can fix it later. Okay. All right. Um, do you want me to point them out individually or just have you look through the style guide and use that to check through it? Oh, I didn't know about the style guide. Oh yeah. I wrote a team code style guide trying to spread it around. Did you not get that when you joined? I'll email it to you. Okay. Thank you. Um, cool. Um, it sounds like the rest of the critiques that you have for this PR are sort of code style level things aside from the big switching the, um, That's reasonable. I mean, Okay. Are these things that could be represented in rubo cop? Is that something that we could like wire up to see I somehow so we don't have to do this by hand? Oh, that's a fine idea. I mean, like, uh, if you are you excited about setting something like that up if you want to help. Sure. I'm up for something. Oh, this is great. Yeah. Okay. Let's go go ahead and try that out. And, um, maybe if it works out for us, maybe some other teams will pick it up. Yeah. That'd be great. Okay. Great. Oh, hey, what's up? Hey, Sumina. How's the conference? Oh, um, I'm, I'm, I'm, I think it's good. You know, Ruby, RubyConf is always a good time. Um, so, uh, I, I'm not sure why you're calling now. Yeah. Yeah. Yeah. I worked on the migration from, or to deprecate TLS 1.1. Um, yeah. I worked on the other, the previous one, right? TLS 1.0 deprecation. That was, uh, that was a bit of a tricky thing because you want to make sure that, uh, we get all those cases. If, if we don't do it right, then it's really hard to diagnose those client errors. Right. Um, and so I was looking for like some kind of documentation or, um, like a retrospective or notes or, or something that we had on the TLS 1.0, um, deprecation. Oh, well, I mean, we thrashed it all out in a big email thread. Um, I could find that and forward it to you. That would be super helpful. Thank you. Yeah. Um, the other thing is, uh, I came up with a checklist, um, for our kickoff today. And so I would love for you to take a look at that. The kickoffs today? Uh-huh. Oh, was there an email about that? Uh-huh. Oh, um, okay. So how about, there's a slot coming up. I don't need to go to the talk. So how about, um, I'll go ahead and just, uh, check, check then I'll look at what you sent me. I'll, I'll see if I have any comments on that checklist. And I'll forward you that thread from a few years ago. Great. Thank you so much. That'll be helpful. And I'm so sorry to bother you while you're at the conference. It's fine. Sometimes that's just how it is. Um, I, uh, you know, sometimes you got to ask the last person who did it because, uh, that's who knows how to do it. Right. And now I had to, uh, uh, check when I was doing the last application, I had to call and find out from someone who had done it before. So all of this has happened before. I mean, I guess so. Okay. I'm going to head off and do that. Thanks, Jason. Yeah. And all of this will happen again. Jason. Hi. Hi. I just saw your Zulip message. Yeah. It's not so good time for the code review. Yep. If it's good for you. Uh, is your feature branch up to date? Yes. I just pushed the latest changes and it's ready for you to take a look. Okay. And have you tested it? It works on my machine. Famous last words. Okay. So I'm going to pull it down now. Last week when we looked at your work in progress, we talked about using Brian's library. How did that go? Good. Um, he was able to help me set up the dependency, um, and add right that first API request, which was very helpful. Okay. So you pair programmed with him? Yeah. A little bit with him and then also a bit with, uh, Sarah. She came over for a couple of hours yesterday and we had a lot of progress. Oh, that's really great. Okay. So without looking, can you tell me what this PR does? Okay. Yes. Um, this is for bug 1104, which is to stop parsing HTML and start using the API instead. Right. Um, and I figured while I was in there, I could clean up some of the code, um, add a bug class to handle some of these interactions. Um, and of course add some tests. Ding, ding, ding. Yes. That is exactly what you say in your excellent commit messages. Thank you. Yeah. Sarah was real big on commit messages. Um, and she was also able to help me break up a big commit that I'd done before. So I got to learn about, um, get reset and get add dash beat. Oh, that's really awesome. And tiny commits are so much easier to review, but you've heard me say that. Okay. It looks like the build finished. It's time to pull that lever, find out if it's all green. Okay. It seems like there's just a few little style guide violations here and here that the linter caught, those are super easy to fix. Uh, we have some docs about how to run the linter locally that are on the wiki. I'll just make sure you have a link. Good. All right. Let me just keep looking. This is good overall. This is really good overall. Um, this test here is pretty well named. I wanted to call that out. You are handling the error case of this API call correctly. And that is one of the main things I wanted to look for. Yes. Um, I was poking through Brian's library as I was trying to figure out how to use it. And I am so glad I did not have to like find and write code for all of the edge cases that this API has. I know. Isn't it great how much detective work he did on that? Yeah. Yeah. And it's not the only one, but I want to show you here. See this comment? He has actually filed a bug with the vendor upstream. Oh, good. Yes. So maybe we won't have to have this hacky work around forever. That would be nice. Yeah. Um, and this plus some other detective work that we've done with other clients, other APIs for other vendors, he's actually thinking of collecting those together into kind of like a case study that he's going to be submitting as a talk to RubyConf. That would be wonderful. Okay. Yep. This is looking good. Uh, let us head over to your desk and fix those tiny linter violations. And then we can merge this. Great. How are you feeling about the code review? I'm okay. Okay. I'm a little nervous. I don't understand that. Um, let me just put you to use a little bit by saying that this is a good pull request. Like I took a look earlier and you did a good job. Um, I was nervous. My first time getting a code review too, but, um, I'm not here to critique you or to like point out flaws and you as a person, um, you are not your code. And if we find a bug, then it's super easy to fix because it's just code. Um, and of course now is the cheapest time to fix it because it's in your head and a little bit in my head. Um, and it's way easier than in like six or 12 months when we've forgotten how all this works and we have to figure it out when it's breaking. Um, and also I hope this will be a learning opportunity for both of us. Um, so I will get to see how you have approached the problem and thought about it and maybe learn a thing or two from you and then you will get to see what I am going to be looking for and asking questions about and, um, hopefully we'll, we'll both get to learn from that. Yeah. I guess that seems like a good way to do it. Great. Um, so how did you, how easy was it for you to make this change? Actually it was like surprisingly easy. Like once I got the liner running locally, I mean, Good. Um, like I was able to use get blame for a lot. Like everyone's commit message was really clear. Yeah. Isn't it great to be able to look through the history and learn from previous developers and their mistakes? The end. Uh, we have a few thank yous. Um, we would like to thank Betsy Hebel for co-starring with us. Thank you to the sound and light cues, Audrey Eschwright over at the AV table. I volunteered. We would like to thank our director, Jonathan Galvez. Yes. Yeah. Um, the, the staff here who did, uh, the, the sound and, uh, the, uh, the events crew from the AV staff. So specifically Randy and Paul. Thank you for working with us. And thank you to the RubyConf organizers and especially event producer Heather Johnson for coordinating with us. And thank you all so much for coming to our play. Enjoy your break. Yes. Have a good day.