GitLab Code Review Weekly Workshop, 31 Mar 2023

Previous Meeting Next Meeting

⏯

youtube image

►

From YouTube: Code Review Weekly Workshop - Mar 31, 2023

Description

In this session we discuss some code review topics and pair up on a code review.

00:00 Hello!
01:00 Finding who has approved when approvers are reset.
06:00 Let's review an MR

https://gitlab.com/gitlab-org/gitlab/-/merge_requests/112892

A

Well, thanks for hopping on the code review weekly Workshop, uh we are gonna dive into some topics pertaining to code review, except neither Yannick or I. Have anything to talk about so uh we're gonna pair up on a code review. uh Is that an accurate betrayal of the situation.

B

It is very much so um full stop well.

A

Let me um start sharing my screen before I. Do that, let me close out make sure I don't have any secrets around am.

B

I dive bomb there's another short question: that's just coming up and it is um given any merge request. What is your Technique to actually check or like on a quick technique to actually check who um already approved this asking this question, because she likely the approved by section that we're having is sometimes lying to me and I'm. Currently looking at one of those things and it seems like it is.

A

Yeah, so if a um based on the based on the projects rules, um it's possible that a code owner that is approved is reset when new commits have been pushed. This.

C

A

Is hey new commits are pushed this isn't approved anymore. This one needs to re-reviewed and approve it, but what could happen is if a back-end engineer has already approved the back-end maintainers already approved this, but.

B

A

Front end code changes the back-end maintainers approval should still be relevant, it doesn't need to be review, and so that's really up to the front end maintainer to distinguish that, um and so what I'll do is look at. The commented by to see because, like most likely whoever's approved, has left the comment, and so, if I see this hasn't even been commented by a code owner for Ruby files, then uh uh this hasn't even commented by a code owner for Ruby files and it's very unlikely it was approved.

A

But I will do this to see who's actually approved. I will control F and look for approved in the logs.

C

A

Enrique approves, but if I'm looking for like a specific one, I'll see commented by some Ruby code owner I'll, see who commented it and I'll look up their name as like. Did they actually approve and yeah.

B

Yeah and with the like, the the control F approached like, let's imagine like a random Mr I would review it. I would approve it off. I would still make some other changes, and my approval would then become invalid. With the um there would be like a log entry that I improved this thing, but there is no entry that, like this approval, has none now go go out of uh state or whatever it is, is.

A

It yeah I won't say that you unapproved it that will only say you unapproved it when you've actually unapproved it yeah.

B

It feels like it feels like to ever be a great feature to enhance this thing.

A

I'll regularly, if a back-end approver was like a week ago, and then we went.

B

A

Back and forth, I find myself with and they rebased or something I will.

B

A

um Compare the changes of like any time someone's done, a big rebase I, look at the diff of this diff compared to last Mr diff and if they're, by back in sometimes there's actually back-end changes uh and I was like. Oh well. We added this thing, we'll need to get the backend maintainer to look at it again, and so you do have to sometimes dive into. What's actually changed between an approval and now but.

B

That's kind of.

A

Like a job of maintainers, so it's like for maintainers is making sure all the boxes are checked and.

C

B

Do you agree I, just um full stop I, just think that every probably like the these things coming in there and what we're seeing on the left is basically somewhat of a timeline um like all the the comments and then the degrees right that is going through. This there'll probably be a great thing that we actually would highlight um for any approval. What actually has been approved.

A

B

A

Guess: here's the little icon! You can look for uh it's a it's very little.

C

B

And maybe even like, in best case, even in the commit history, even though that that won't cover a rebase or something and squashing and all of these things. But that's that's about hygiene, I, suppose.

A

Yeah, so you can just you can just for this. This SVG approval, that's it I'm. Joking yeah.

B

Sweet okay, that was that was helpful, yeah um now off to after UMR what'd you get.

A

Yeah yeah: let's do it um yeah. That would be cool for me to if I could somehow organize these. That would be interesting.

A

um That's a good idea, all right, so this is actually um I was wanting to show this one, because this is actually part A, Part clmr.

A

So the target branch um points to Source Branch points to a non-default, Target branch, and so this is a different Mr and then this one is just iteratively building on it, which is a great thing, but one of the things that's interesting here is when I try to do my.

A

um Reset changes here when I try to do my curled Mr diff, get why it blows up, because the master Branch doesn't have the changes that this Branch does.

B

um Can you quickly put the link in the chat I want to see if, yes,.

A

I think I put it in I, put it in the agenda, but okay.

B

C

Thank you all right see. That's it.

B

A

Yeah doesn't make sense, though um yep, um so what I'll do is that's when, rather than working on top of Master and applying changes on Master I'll actually check out this branch uh and then apply the Mr diff. On top of this one, so I'll actually check out that.

A

Target branch and get the latest from the target branch and then I can throw this thing.

B

C

B

Is kind of the same thing but different you.

A

Know yep I mean, and so it's just I think something that you don't realize most normal Mrs that aren't touching other branches. Is they go to master, and so, when you're, just always working on Master, you can just easily just push everything on top of Master, but sometimes you're reviewing NMR that doesn't touch Master as a maintainer I'm not going to merge this Mr. This is just reviewing this iteration of code and assuring oh yeah, we're all good um the.

B

Problem um the question Christian regarding the process of this, like, let's imagine for a second um changes in here, are all looking perfectly fine you'll. It gets all the approvals and um actually gets emerged into the um into the feature Branch listed in there.

A

So it accidentally gets, you said accidentally gets merged into the eventually I.

B

Said like on purpose really: let's just like: let's um process is going just fine everything's perfectly fine and let's merge it into the other feature. Branch listed in here. This feature, Branch I suppose will get get or should be, shall get merged into Master someday. Is there any like this? Diff will be huge? Is there any indication that the changes have been reviewed um before so.

A

That's a really good question, so I think the I think the actual approach is um and I don't know. If David has left a comment about this. Okay I'll leave a comment about this, and so, rather than merging everything into a big feature, branch and then merging that feature branch which uh which I've done and I don't recommend.

A

um It is don't merge the SMR until.

A

um The one before it is merged, yeah.

B

I would like that would have been my Approach as well like you can do this in, like as kind of a draft mode. Thingy, yes um and then keep on rebasing your Mr, that's okay, but um merging them all together. They will just create a massive if.

C

A

C

A

Is the preferred way of of handling it and, as the Mr author, that sets up a chain of these on Mars I'll usually have? um Let me see if I can just find an example.

A

This is, this is probably an example. Yeah yeah, so I'll usually have um so I have my part, one Mr and then part two points to part. One branch point three points part two Branch, but.

C

B

A

At the top of these I'll put a.

A

Oh, what am I gonna put, what do I put oh well, I can't find it, but anyways I'll usually put like a to-do. Don't merge this like we gotta wait for this other thing to be merged and we'll rebase, free Target, and um but what I love about doing it? This way is the changes are cohesive and small I'm able to vertically slice my iteration so I'm, not just horizontally slicing, like here's.

A

One huge component deal with it, I'm able to see how everything works in integration but grows together, um but I can get these reviewed in parallel. I love that and that's a great um that's a great uh productivity boost when, if I put all of these into one Mr, it would be incredibly daunting and it would take a long time for someone to process it all and feel okay about it. But if, in reality, it's three straightforward changes which someone upon looking at it will be like. Oh yeah, that's great: let's do it!

A

Man, that's gonna, go by so quickly. I just can parallelize all the reviews and then just.

C

A

There's just wait on it, and, and uh so surprisingly, I could see how splitting up in Mars sounds like a lot of work, but it really helps things. Go faster, yeah, awesome, but.

B

Anyways um sorry go ahead: no! No! What we're gonna say, reporting that I have not yet been that brave, like I split up Mrs I built Mr chains, but for the reviews then I would kind of zerilize the reviews, basically being like okay, let's wait until this thing gets approved in the established yet but yeah you can with a couple of explanations, you can probably speed us up.

A

I want to see yeah I want to see if I can find it up. I'm gonna see if I can find another one. Let me see if uh this this one might be. One too. This is on the web ID project yeah. It was only. It was only two parts, uh so it's not a good example, um but I'll this is so. This is actually how I normally do. Things is I create one gnarly Mr of like this is my dream. All the changes I want to make and then, from that I pull.

A

What's the change? What's a what's the smallest change that someone looking at would be like? Yes, let's do this and.

A

So after making one like huge gnarly change, I'll uh I'll iterate over that- and that's that's so helpful to me, because I really need to see it all sketched out kind of before I can detail. You.

C

Know what I mean? That's it.

A

uh Okay, so we're on this we're on this similar, which uh uh David, if you're listening it would have been cool to have some some cross-reference links to the previous Mr, because I can't seem to find it. But that's fine.

A

And I checked out the stuff locally by checking out the target branch and then applying the changes from the MR here. So I can review the changes uh which I love doing so. One of the suggestions I left earlier was previously. We created the Apollo, client and stuff just outside. There was no wrapping function, it was just part of the module and I and I just said: hey normally like we try not to use side effects in modules on import.

A

So if we can wrap our Apollo client creation and a function and then call that in our initializing function, that'd be great.

B

Yes, before we jump into code, can we just one with one sentence, speak about what the someone was actually about, because yeah.

A

That's a great question, um so this Mr- this is the back end. Stuff isn't done for this, so this is just once very small step of the UI to hook into some mock data.

B

Oh okay, gotcha.

A

So that's where you see it, I mean this is some really interesting code. It's like this is like test code. What's it doing here, but it's like hey this. You know this helps build out the front end in parallel and very iteratively, along with the uh back end, while it's all under a feature flag. It's like yeah put in mock data. That sounds helpful right uh-huh.

A

What do you think.

B

It's uh yeah I do um I, do the same thing. um What I do is I, usually I, don't even know why I um I usually don't commit mock data, but what I do is provide a patch to values.

A

um That's really interesting.

B

Yeah, maybe like it could depend on the use case whether this thing is possible or not, but, um like my classic, my classic use case is there's already an existing endpoint, but it will be uh from the back end, be enhanced with with like a super set of data, with more data coming in that we usually have so um yeah I would mock this, provide a patch um so I'll leave the the existing code untouched and not to to have it any yeah, not too little or anything um but being able to have you a mock.

A

Gun I see um I like that approach, I'm I think as a reviewer like I'm fine, either way if a team wants to commit mock data while you're building it out and if that is helpful, like sure like, let's go for it, if patches are more helpful, then yeah, let's go for it. Hatches cannot. Probably I could see how that may allow you to be just provide so much data, and you also don't need those patches reviewed. It's like.

A

Oh, we changed something with the patch, so it's like here's where the patch lives and like so all of that I can see how that can have its benefits, but see kind of. Do you think this is with the is this something that you would leave up to the author's discretion and the team's discretion? Or is it something that.

B

um Yeah yeah I would um I'm not too sure how bold I would want to have a comment that what you're currently looking at is mock data and supposed to be deleted, like this is uh attention attention. This is uh not ready for for protection, and that might be something, but in theory, I'm always used like under given circumstances. It's okay to commit mock data. Yeah um I personally, would probably agree a patch because I don't have to clean up afterwards. That's probably.

A

Yeah, that's an interesting that is an interesting point. um Yeah uh yeah! That's a that's an interesting point.

A

One thing with this file that I'm noticing um so related to the mock data.

A

uh So this is round two of the review and I'm, anticipating that this is all gonna be okay, but um on The View component itself, pagination was implemented a little I, don't know if it was actually gonna work um and, as you can see in our mock data, we're only returning one item. So it's not like.

A

We even had the mock data to test out pagination, um so I kind of just suggested with this at this level, let's just remove pagination until we can actually test it out, um but I see that uh the page info.

A

Part of the data has been left behind, so it's not part of the query anymore, but it is part of the the data uh which this is not a big deal, because this is all tests code. At this point, and uh as long as it doesn't just totally break things and make things inconvenient.

A

But I would could say something like we're not recording for page info anymore. So maybe we don't need.

C

A

Something like that, um all right, let me go ahead and start the GDK so I can uh we can test this out um all right. There's our cool little Quarry.

A

All right here is where we're making some.

A

Let's look at this file.

A

Let's look at the template first, so we have an alert. If there's an error message dismissing goes to clearing the error message boom workspace as an empty state. If we don't have any workspaces, we're going to show the empty State else, this means. If we do have workspaces, then we show our little header and if we're loading.

B

You want me to say something that yes made me raise an eyebrow yes and 995, the D minus 2x class. ah What's that going to be.

A

Ah That's that's bootstrap Flex ah um yeah.

B

We don't do that, like you, buddy I, know who.

A

C

A

Are you, why are you hating on our bootstrap we're trying to slip into the products yeah.

B

What's what's wrong with that, yeah.

A

We'll have um that's a good call. um I I want to oh flip-flops uh I. Don't know why it's telling me I want to learn the basics of git I'm a software developer, but I want to learn git cool.

A

What is get lab.

A

All right, let's sign out sign back into I, was clearly testing out system. Headers and Footers.

A

All right here we go. Oh look, it's the super sidebar, um so you were right. We want to.

A

Why do we even have this as flexed.

C

A

Not seem needed at all right.

B

um Yeah you're moving slightly too quickly for me too, but um it still seems all right.

A

um So this is where the D Flex was in the app and so now I'm thinking we don't even need this.

C

A

Change anything doesn't change anything.

C

B

Like oh and then there's a table Yeah like oh okay, now I get it um yeah like it really seems yeah. This is not great anyhow, but these classes are not going to change it.

B

um Okay! No! So it's only um only the heading in one container with the table underneath so yeah you can give it a flex, but you can also not give it a flex like it doesn't really yeah.

C

This doesn't use.

A

Yep all right so I'm gonna leave that as a comment, good catch, really good catch. I. Think part of me has started just gloss over utility classes and it's just like uh there's.

B

Really, there's really a lot to be found.

A

I know I know there is on those things.

B

um Just just the fact that we probably would have just like if this would have been a gfx message as well.

A

Yeah I wouldn't have even thought of removing these if it was all GL classes, I was like sure, that's great.

A

To try not to use bootstrap utility classes like the flex um looks like all of these classes might not be needed.

C

A

Since there's only one.

C

C

A

Okay, one interesting thing I'm noticing is that uh we won't be showing the loader unless we have workspaces.

B

hmm That could be intentional, but it doesn't have to.

A

Right I want to um I want to force the loading state somehow.

A

I, don't know how to do that.

A

How can I do that.

A

Man I, don't know how I'm gonna do that. Maybe I could do that in uh The. View uh browser thing: the.

A

Do you think that's possible and- and this.

B

They'll, probably be painful, I would argue, the updating state in there is not, um let's see yeah I was expecting it. I I hate it like somewhere, really really really at the back of my head. um I feel like it is possible just.

C

B

Clicking in there to update States somewhere like there is a trick, but I just can't recall it all right, all right, yeah, it's not! Let's update the component, I I'd argue all.

A

Right, that's what I do, let's, let's do this, then, let's um I'm gonna, just open up this file now.

A

All right, that's not how code works okay, uh so let me replace the apollo.loading thing with: is loading and see what happens.

B

And nothing's loading, oh.

C

B

A

Right, so, if we're loading.

A

Something tells me this: isn't this isn't happening the way we anticipated to happen.

A

ah And I, don't even think geolite table light even supports a loading bank here, so we're using GL table light I, don't think it supports a loading.

B

um I do agree, um I would need to double check. Like um reason reason just is making me laugh, is um I recently had a problem. I try to solve um where it's real table light actually was the solution for and I asked and gitlab UI and people pointed me to to your table light, um which I did not have on top of my hand, but it turns out like a year ago or something I actually introduced this component, so that was yeah. That was kind of funny. Oh.

A

Nice, nice.

C

A

Right, I'm gonna, uh so we're gonna have so what I'm changing right now is. We need to rather than testing has workspaces here. I think we test if we're empty and empty needs to include. We don't have any workspaces and we're not loading.

A

You know what I mean: yes, okay, so then, if we have workspaces or we're loading we're in this category, which we can actually rather than doing a div, we can make a template.

A

uh We don't need to wrap it in another thing um and then what we'll do is if we're loading.

A

And this has got to be V else right.

C

A

A

Not have the table right is that how we do it.

B

Sounds good to me.

A

So I still have loading reloading is forced.

A

A

Oh no I broke it all.

C

A

A

How did I break it.

C

B

Did I do um not rendering something? Let's let's reach.

A

Out no I broke, it.

A

List we have is empty as false and we're loading.

B

They're not even um rendering the headline, aren't we so the wait, what it is empty, but if there's no headline, then we would be playing in line 95, uh yeah 95. Isn't it.

C

B

Can we can we see this component in the um in the view inspector or say any error on the console.

A

No errors on the console: okay,.

C

A

It's it's just not rendering anything, uh because here it fails. This fails this and I would thought it would do this. Maybe I don't need to wrap this in a template. Did I do.

B

A

No, it's not closed. I didn't close, my template. Oh my goodness.

A

Sorry, everybody yeah! You see this cool vs code. uh Tomas uh Vic showed me this. You see how it's it's telling me where I am in the code with this sticky header at the top.

A

C

A

Nice um there's a cool setting. You can set there.

B

Okay, sticky, sticky, dumb ornaments yeah.

A

All right so now here's it loading, we don't there, it is loading. It's.

B

A placeholder, not a spinner, isn't it or like? Is that intentional I don't think so.

A

I, don't know, I, don't know what kind of loader we usually want to use.

B

A

But yeah there was.

B

A

All right, let me um let me take a screenshot of this and that's kind of maybe a question to ask, um uh but then let me change this back to I gotta go this apollo.loading.

A

Okay boom and we're back and so then to test the other state. If we don't have uh any of this stuff.

A

Are we still showing the empty state.

C

C

B

A

Cool all right.

A

So let me go back to here and I'm going to leave this just as a.

A

As I think as a question of I'm, not sure if we're handling loading correctly and here's I think what I would anticipate it being.

C

C

I'm not sure if we're presenting loading correctly.

A

uh Are to test when this free is loaded, since the data.

C

Data is hard coded.

A

Not sure if we're presenting the loading correctly, as is we show the loader above the table,.

A

If we were refreshing.

C

Or loading a different page do.

A

We mean to show the loader instead of the table, here's a patch for what I'd expect.

C

A

Yes, so this hasn't some handful of changes, so I usually wrap it in a details, thing.

A

I hate having to type all of it out, though I wish the lab was smarter at handling patches.

C

A

A

It's hard to test when this query is loaded, since the data is hard to remove the computed. This way I could set. Is loading.

C

A

I could loading to true the past.

A

um Then the other thing is, it looks like jail table light, doesn't have a loading.

B

Prop, um let me create a double check that.

A

What do you think, I? Think of being the bootstrap Docs.

B

uh Yeah good call, I mean I took a wrong turn. You're you're faster than me.

A

B table light um here it is properties if I J, K L, there's no l.

C

A

That's how it goes all right.

A

All right after this patch.

C

A

Right, if we're loading, it looks like this.

A

That's the screenshot I took.

C

C

A

Is this for skeleton Lions? What we expect is this: what we normally use for table loading.

A

uh Do we need ux here it's my question: does that sound good, okay, cool um one last thing uh that I'm I'm seeing um Also? Let's keep going, we were here so we pass in the fields and we have the items we saw that that works, and then here's these different templates um looks like when we uh man yeah. If you weren't here, I was just blowing through the utility classes. I wasn't even looking at them.

A

I was like I, don't care if these utility classes are all right, but we're aligning icons, something you might be familiar with. uh um Lex Direction call. Oh yes,.

A

I guess because yeah yeah I guess we won something like this.

A

Oh so these are then all stacking on each other. Okay, that's cool, interesting.

A

We have an item text Gray.

A

Gli and items okay, icon is a branch there's the branch name great.

A

Ahref Target blank.

C

B

It's good to me, yeah.

A

um One last thing that I see is um for formatting the date we're doing this time ago and I I.

C

Don't think we use.

A

That as much I think we have a new format. Date thing. You know. Oh.

B

um Yeah, okay, but like it's um yeah, no okay, it is one of our shared utilities, so that should be um okay.

A

Yes, yes, so I see how it's confusing, though we have date time utility and then we have date, format, utility.

A

um I think we actually just put it in here like this. What do you think.

C

A

I, look, you look like you, don't like it.

B

No I think I like the first version better but I'm, fine yeah but I'm trying to find an argument to uh and I can't really can't really think of. One.

A

Okay, bro the reason the thing I the issue I have with this is that this actually has nothing to do with last. To use that like this is just about formatting a date as I have nothing to do with the last used language.

A

um So let me see if formatting the date works here.

C

A

A

ah Okay, it's not doing time ago.

C

B

So it's just a different different thing: kind of I.

A

Guess so um what I found yay I guess so? Okay, yeah all right to me: it's not a big deal and we don't have to fix it here, but um I would just oh man. I went back too far, I'm just gonna undo all my changes but I'm either not going to make a change I'm just going to ask I'm just gonna. Do a little non-blocking Polish thing of um this method actually doesn't have anything to do with.

C

C

Yes, formatting I mean maybe we want to do all of this or not great.

A

Okay, great um so moving on, we uh renamed this spec we put in a different folder to align with the component, and then here is the spec for the list that we have um so looking at the spec that we have here empty state with no workspace service shows the table when workspaces are available, displays user workspaces correctly row find component, ahrefs and stuff.

A

um What do you think man.

B

Looking solid I'd.

C

B

Okay, I, don't yeah nothing, nothing to uh to mention from my side anymore other than the single mentioned.

A

Okay sounds good I'm gonna um test out the uh the coverage of this.

A

um So let me close this out by removing some template stuff and seeing what happens. Good good call, good call. That's that's!.

C

A

So yeah we're not really testing the loading State.

B

Wow I I keep that with me they're so clever, because.

C

B

But I do have the same question on my revenues: I, usually really not not necessarily line by line but kind of have like tests and codes next to each other and check um what is actually being covered um just more or less randomly removing things seems much more efficient and waiting.

A

B

A

A

Yeah, there's a lot of um uncovered. There's a lot of there's a lot of ways that we can. We can approach this um I'm wanting to I'm wanting to to not be neurotic I can get neurotic about it, and so, knowing that about myself, I'm trying to think what would well with someone who's not neurotic do and I'm hitting a wall, um but one of the things I'm tempted to want to leave. So to me all right. Let's, let's go way back.

A

The uh template is a great place to start with what is covered in intestine or and not in view components. So that's all great.

A

um The other thing I'm realizing I can get really neurotic about is uh I have a way of like testing tables that I really really like, but it might not be really intuitive, but it I end up creating a finder that yeah we find the rows, but then I like serialize the rows into a data object.

C

A

I, like create a very fancy finder. It's like okay for each one of the rows.

A

This is what we expect from this one we're going to find this component, and so then I have like an object that it pulls from each one of the rows. So I end up with like an array and I can actually just say, find table rows as data equals mock group workspace nodes or something like that, so it's like I rather than just saying oh, was the text contained at all, but I think this is fine. uh Well, actually now I'm looking at it. Oh, we don't usually do um expectations in like Loops like this.

A

Oh yeah, yeah, yeah, so I think I am going to suggest that we do this thing as well um all right and then and then we'll. Then it's time to go uh well, it might be time to go right now. What do you think? Do you get a head to pairing time? Probably do we gotta be on time for meetings? It's important yeah.

B

Yeah I mean I need to run. There's nothing for me today.

A

All right, well yeah, so I'll I'll leave this small little comment but yeah. Then we reviewed this thing together and that was fun yeah. It's a it took a while because we had a good conversation about it, but that was good well.

B

A