GitLab Code Review Weekly Workshop, 6 Jan 2023

Previous Meeting Next Meeting

⏯

youtube image

►

From YouTube: Code Review Weekly Workshop - Jan 6, 2023

Description

In this session we answer some questions and talk about some tips pertaining to Code Review.

We also get to pair review on an MR.

0:00 - Discussion
42:00 - MR Review

A

All right, let's all right um so hey thanks for hopping in the code review, weekly Workshop, where we talk about uh code review.

A

um If there's some things you want to show and tell that have been interesting, that you've been using while you're reviewing code or things that you think about or look at or if you have any questions pertaining to code review. This is a great place for us to synchronously talk about it, publish the recording and we can continue the conversation asynchronously as well. But then we also have a really fun time where we get to pair on a code review.

A

If anyone has a code review in there um in their doc that we can all hop on uh and pair similar to how we do with the front end pairing it back in pairing.

A

I've got the first item here uh where I've been using uh running tests locally with coverage, and this has never worked for me in the main gitlab projects, because I didn't know about uh this option where I can say, collect coverage from if I just ran yarn run just coverage, Foo spec. It would try to collect coverage from all the files and show that we're not covering anything because you're just writing one test, but it actually works if you specify collect coverage from so what it would look like. uh Let me open up the code.

A

uh I was reviewing an MR over here, um but what it would look like is so here's the I'll close these other tabs uh would love to know if there's something similar um uh to this in Ruby land, uh but I would run coverage. That's not how you spell coverage, select coverage.

B

From is that what it said.

A

Yeah and so then I'm gonna paste in the um subject under test and then I'll pass in my main test here. Hopefully this all works.

A

All right, cool, so I think there are some assertions that failed, because I was poking around at this, but you can see. Okay, we got you know pretty decent coverage. I haven't been able to on pre on other projects, I've been able to get it to spit out this really cool table where it actually shows me the lines that are uncovered and I think you can actually jump into that. Specifically here uh you can um oh geez Louise. Where is it yeah yeah?

A

So it actually like spits out this report, I've been able to get it to where it spits it out on the terminal, but for some reason, I can't figure out how that's why that's not working in the gitlab project, um but you can actually open up these index files and see the actual lines that are uncovered, um which is interesting. It's definitely interesting for I think when we introduce new files, I like using this just knowing like okay did we do. We have we established a pretty good test harness for this new file.

A

It's a lot easier for me to understand that question uh than just parsing through the all the unit tests, which you know oftentimes when unit tests end up a little bigger than our main subject under test, so uh I just been finding that helpful, not only use it all the time, but in certain situations running the coverage locally. Just for that one unit test has been helpful for me. uh Yeah do do you all backenders know if there's a ruby equivalent to doing this.

C

ah There is um I have to look at the docs. That's.

A

C

A

C

In the documentation, yeah there's some way cool.

A

Yeah does knowing that it exists, uh will encourage me to look for it.

C

Yeah sometimes I rely upon the little colors in the Mr uh diff, but those aren't always there.

A

C

I think the coverage report is not always finished. That's.

A

Why that's? Why I do it because I don't know when and when that's not working and yeah.

C

Yeah and you don't want to wait for the pipelines to finish yeah so, but it's definitely a way because when you have an issue with the coverage report like you want to be able to test that locally to see if you fixed it. So there's some something somewhere in the developer guidelines about how to get it to run.

A

Okay, yeah I'll have to check that out. um This was really helpful for me when I was writing code um on the new web ID, we had to implement a file system interface interface had like 30 plus methods. It's like really low-ish level that you'd imagine from a file system interface. It's like nodes file system, thing and I could have um I could have tested this by each function. All of the inputs like did.

A

We do all the stuff that way, but there are specific use cases we're really interested in so I was able to run the coverage report and get it to tell me what lines were uncovered and come up with use cases that I could add like more almost integration level testing at the top that were actually like more helpful than these, like did I write the function that I expected it to write.

A

So that's my favorite way of using it too is coming up with the use coming up with use case tests that cover more of the behavior of the specific lines you're missing, but obviously 100 coverage is also like. That's a myth and like so you gotta, you gotta, you can't get tunnel visioned on it, but it's uh a helpful thing to keep an eye on cool thanks for letting me share that.

B

um One small thing: that's just crossing my mind, while you, um you show those things. This would be a perfect request, widget for Upward uh product, wouldn't it like. We do have tests coverage for highlighting it and a different stuff. But um when it comes to the 3D Mr I guess it would be great for any kind of this being like hey test coverage, dropped, increase or whatever it might have been.

A

Yes, I think so too I just don't for some reason. It's there I feel like I've, seen it and then it's it's disappeared.

C

I thought I had it too or like I.

B

Know not maybe I feel.

C

We had something that was about coverage, let.

B

Me, let me just paste it in the agenda. I just looked in our docs and that's what I found just put it in there, um but from what I get it's only in our diffs.

B

Yeah not so much in the Emma are over. Here doesn't actually be sorry. No.

A

Sorry um I was gonna say that I bet if I was um I I'm, not a very I'm, not a very wise developer of my time. uh I get nerd sniped easily of like oh man, I can run the coverage.

A

You know, I get tunnel visions and nerd Snipes easily uh uh I think one way that we could do it is we have a coverage job or these just oh I, don't know if these, oh, no, we probably do per just job either way those static assets will have the like, probably have those HTML reports that we could actually open from the job the job artifacts.

A

So that's I, think one option for our project is those HTML files that have all the summary might actually be job artifacts that we could look into as well on one of the jobs I would imagine, but yeah thanks for sharing that Yannick.

B

Yep uh very last thing on this that this will actually be this in my team, or at least not very close. So the next Outlet meeting I'll just ask people who know our Pro better than us and we'll see her ticket.

A

B

Yeah, let's read um I put the the next point on the agenda. It's a little trivial but I was amazed how how much benefit that actually was in for me this week. That's what I'm, showing it's slightly related to Mrs. uh It is very related to Mrs, not so much well, we'll see about reviews.

B

So um what I've been working on this week have been a couple of small changes that were all very much related to each other, but still different enough to justify different Mrs, because they'll be going through ux reviews and all this this kind of process you I, had several reasons.

B

I didn't want to end up with a with a massive Fairmount, um but to make my life easier, I actually built this this Mr chain, basically starting out with the the very first Mr and then branching out of it, and setting um setting my first created mrsd Source Branch, where I was at the beginning. A little bit concerned with this might bring me, but it worked out perfectly fine. You just have to time the reviews you're getting in a little bit and not confuse people, but for several things.

B

I might stick with this technique because there's a there's so much things you'll be actually solving without rebasing, not not running into any conflicts. Just for developing experience, I really really enjoyed this. So I was wondering um anybody of you doing a similar thing. Do you have any workflows in place, or is this something you not really do in your day-to-day.

A

Are you talking about having, like a part? One part, two part three Mr, and these are part three is based off of part. Two's Branch part two is based off of part one's Branch, yes, yeah, exactly yeah I love that I. Do that all the time.

D

um How do you deal with it? How do you deal with the maintainer reviews on the final Mr? It means that the final Mr will be probably huge, and you still need the same person that reviewed the previous one, so they have a context or.

B

um I went the other way around basically having my first iteration, which was a small change with viewed and maintained and merged, um and while this was still that was the very first thing. um So how do I put this?

B

It actually allowed me to keep on working while the values were still in place, but um it was not that I merged all these blocks branches together and put it in one review. It was rather splitting up my work and then actually have it reviewed four times you would say, but yeah just split in chunks. So that's how I dealt with it.

C

Yeah I guess I've been doing something simple more, but I try to get um the ones that I've been doing or where I can like merge the smaller pieces in first and then base the later work of the smaller pieces. uh What I keep running into, and it's not it's just a little bit of an annoying, because I have to do like rebase onto as the the top Mr merge into Master because of the squash.

C

So often like you get a review and feedback, so you make some changes and then that gets squashed and the commit hash changes which makes all of the later Mrs now look like they're changing more files than they actually are and I find that to be I, don't know, I was a reviewer. I would be very confused, so I often will go and take.

C

What's now the top Mr and do rebase onto like go to master, find the new commit hash and then kind of just like redo everything, I I, don't I, don't know if there's a better way.

A

C

A

Have you ever considered like using fix up commits instead of well.

C

I got feedback from someone when I first started working here to like not do that. Oh.

A

I'm waiting, I'm.

C

Waiting, you said it made it really hard for them. Yeah, so I got feedback from like a maintainer that they didn't like it and so I stopped using it.

A

C

That means that prevent.

A

C

A

Unfortunately, like gitlab's UI does not support fix up commits as well as it should, um but if you only have one commit, uh you just want to make sure you check that hey we're going to squash before merge and then.

C

A

Time you push it'll change its draft, so that's the other weird thing, because you need to make sure to every time I push a new fix up, commit I have to undraft my Mr, um but I use it all the time because it helps reviewers to know okay based on this last round of changes. This is this. Is the latest commit of the changes, the immediate changes and, if I had more branches pointing to that I? Don't have to do the I?

A

Don't have to do all the rebasing stuff, because the Shaws haven't changed um most to me, it's just another way of renaming people use effectively fix up, commits by titling a commit code, review feedback, uh yeah.

C

A

Effectively, you should just call it fix up, um I think the issue is we just don't want to merge, fix up commits, and so when we do squash before merge, which most maintainers are pretty mindful of of of what is the commit history of actually going to be merged? uh It should be fine, um but I'll I'll find myself doing these like multi-part Mrs and not having to rebase as often because I'll do it incremental fix up commits when I get feedback.

A

If I do like a big breaking change, then yeah I'll have to then I'll do a squash. Whatever else I need to do, but um one of the things I'm mindful of is on round two three plus of the Mr.

A

The reviewer has one set of changes that they that are fresh for them to look at, so they don't have to look up the whole change that again.

C

Yeah that I definitely yeah I definitely feel that I feel.

A

Like that's so interesting, you got feedback not to do that, because we actually correct me if I'm wrong to tell you I, remember, mark telling the front Enders really try to do this.

D

Yep we had it: okay,.

A

D

C

I, don't know I, don't see, people use and I'm gonna talk in the back end, there's a few here and there, but I feel like most folks when I'm reviewing things uh they'll just leave a little commit message that says like review comments, fixes from the maintainer review or something I.

A

Wonder how many like con, but.

C

Yeah I mean I'll, give it a whirl if I get the feedback again I mean I feel like to me. That seems a little bit like personal preference. When I was first working here, I'm like I, don't know like if it's, if it's making it hard for the person to review like I'll, be happy to change it, but I don't know. Does the fix up mean that someone can't review it right? No.

A

I mean that's. That should just be nothing but helpful right because they could click on I'll I'll reply, I'll say what what I'll say is hey back to you. I made the latest changes in the latest fix up, commit and so they're able to go to the commits and see just the latest changes are going to fix up if I had to do a rebase or whatever else we had to do like that's I find that helpful, um but I don't know um that is interesting. Feedback yeah danger, use. I.

A

Imagine danger was probably complaining at some point because it wasn't always able to detect if you were going to squash or not um so that might have been relevant.

C

But that's a good idea to try to try again.

C

I mean a little bit more, so maybe I can give it a whirl. Like you forget, I joined in like March 2020. That summer was a really interested like the whole year is just not. It was a wild time. So.

A

I think at git lab how efficient we are and how much you know things get done. It's like it's like a month or two and then you're like okay, you're part of the furniture. Now you're yeah. You definitely get to make your own rules. If you want to use fix up, commits, go for it. Terry uh cool.

A

All right did anyone else have any other uh tips or questions they would like to share pertaining to code review.

E

E

D

Do people feel a lot oh yeah.

E

D

A

E

Awesome I wonder how do people feel like about uh like putting lots of stuff putting lots of uh code into like, which is one feature um into one Mr currently I'm working on a it's kind of a fix up, rather than the future. It's kind of like fixing something fixing Precision on one of the columns.

E

um And so I have three migrations um and I put all of them into one of my and um yeah. How do people feel about that? I kind of feel like it could have been split, but then the the uh kind of I don't know I feel better just putting them into one or more making making sure that they like go after each other, although with the timestamp, it wouldn't be a problem anyway, um but I. Just wonder how others would approach that.

A

That's a good question. That's really related to what Yannick was talking about too.

C

uh I feel like I have seen like related. Are you talking about database migrations, yeah,.

E

C

A related database, migrations I feel like if they're related, I I have seen them in the same Mr.

C

um It's like you can have both sides of of what you're looking at, where it's, um if you're as a reviewer I, feel like having them in the same Mr might help because you'll have context of what you're reviewing and you may not have the same reviewers and maintainers on the same thing, um but balance that, with like a very, very large Mr and I, don't know, I have seen sometimes on database, where I've asked folks like.

C

If there's any way for you to split this up, please do no one ever does, but I've always feel like I'll at least ask.

C

Could you please split this up if possible, because it's too big um when they try to put like database changes in addition with back-end changes using the database things and front-end changes in the database, things like so for that that's more problematic than having a multiple migration Mr, but I I am not a database maintainer, so I'm a reviewer and most of the time, I feel like I'm trying to do my best to review things and the maintainers always find more stuff. So.

C

um But if you haven't gotten feedback from the maintainer to split it up, I feel like it's probably fine,.

E

um And another interesting thing, um thanks Terry by the way. um Another interesting thing is um the recommended reviewers, you know how like we get the table with a couple of names um and and depending on what kind of changes you make you get the recommended reviewer and so for this particular Mr, which only has migrations.

E

I I got only like the database reviewer and database maintainer, no like back-end, reviewers or back-end retainers um and I'm kind of going with that, but I kind of feel that I would I would be happy to ping someone else from my team, like just a back-end engineer, um but I wonder how people feel about that. Like I would pink more people than it's recommended. It's.

D

Completely fine to do so because we also have someone called the main expert, at least on the front-end side. We do I think we do have on the back-end side and for your team, the main expert most likely iot members. So it's absolutely fine being them to ask for their review to ask for earlier review. For example, we do this on the very, very early stages, when it's not even ready, but between just a few people to take a look if it makes sense to go in this direction.

D

For example, maybe it's just like full stop and switch to something else, so don't be afraid to think more people then, or let's recommends, and if, for example, if you lack some reviewers most likely, the reviewer or maintainer will say so. So it's like I would also recommend picking someone from back end or in the front-end kids like I would recommend pinging someone from ux to take a ux review. So don't worry about this and don't don't be afraid to Ping people. Awesome.

E

D

C

Yeah I'm not sure how other teams work, but um on the search team we are often because our team deals with elasticsearch and open search and there's not a ton of folks assuming expertise in those areas. We will often pick uh the initial reviewer just from our team. That way, the maintainer can have more confidence that somebody with that domain expertise has taken a look at it and it kind of also covers, like the back end review.

A

That makes a lot of sense. That's a good example.

A

What'd, you say what'd, you say Casio.

E

uh A follow-up question so, even though they were not recommended as like people to pick you've like picked outside of that scope,.

C

Is that sometimes I just ignore it and pick who I want you know? Sometimes we all do I.

A

Never I never interpreted danger, Bots the inverse of danger. Bots recommendation is I, don't recommend pinging. These people, like dangerbot, recommends these people. It does not recommend anyone else. That's one way to interpret it, but I, don't think that's what it means. uh The one yeah, the one thing I would I would suggest um if you have like a back-end domain expert and you want to get their opinion as well as wanting to get it maintainer reviewed um it's for efficiency's sake.

A

You really want to assign one person to review the back end or review the front end at a time. Otherwise, if two people are assigned to it, no one's probably going to pick it up or they'll start conflicting with each other, which can be uh challenging. So it sounds like Terry has.

C

I will say that yeah. That is no, that is, that I've had I've, had database review because I'm only the initial reviewer, where they've assigned me and then like other database people who, like clearly know more than I, do and so I'm kind of just like what do you do? In that case the you know, I I go on this Mr and there's like 68 discussions already where I I felt you know. I cannot contribute to this anymore.

C

At that point, I think I said I'm just gonna, unassign myself, it seems like you're getting like help and I don't feel like I can really like give anything else, but I'm just gonna watch what happens, and you know maybe learn from this, but yeah. It is hard, so I will plus one to just assigning the don't, assign the same like two back interviews, the same time or.

A

C

Of the same first type of reviewer, it can be confusing.

A

And and doing what you did Terry makes a lot of sense and just politely excusing yourself if you're like hey, my presence here is not efficient, so yeah I.

D

Would add you can assign to people, but you need to specify very clearly what do you expect from them, so sometimes I'm being assigned to the Mr that already had a reviewer and meditator, but I've been a very specific type of the problem so like we are stuck with Apollo caches, for example, could you please help us with this little part, so I'm not looking at the Mr I'm, just looking at what I'm asked to do for, but so, if you're assign multiple people give them tasks like I expect you to do this.

E

Yeah, that's a good idea. I actually heard it uh I, don't know right, like some people say: yeah assign a few people, because that will speed up the code review and now you just said like probably better not to because uh they don't know what they should do or like there is. There might be a conflict, so it I heard it both ways.

C

I wonder if they mean like I will assign multiple people for review, but I gate like I need an initial front-end review. I need initial back interview. I need a QA review because uh for the efficiency like I, don't want to wait. Often.

A

C

Not gonna lie like I, don't always look at the front code if there's front of code in an MR that I'm reviewing I trust that the other Mr reviewers that were looked at it I'll like glance at it if I need to, but I'm not going to go digging in view. Hopefully.

B

C

At this so like from that aspect, I will assign like three people at once. You know what I.

A

Said I look at the back end code. What are you saying, I.

C

A

uh No, it's definitely sometimes it's hard, but it is hard yeah. No, don't don't worry about it, but yeah. That's for efficiency. Sake of like hey. You shouldn't be expected to be an expert in something that you're not claiming to be so don't be efficient about. It is definitely uh the goal um yes and Terry.

A

You're 100 correct you should assign reviewers um across the stack to parallel them is great idea um and as maintainers and reviewers, if an author hasn't done that, like the bias for Action, if we see hey this Mr needs looks, SMR needs to go like, let's, not just let the off. Sometimes authors need. You know some coaching from example of how to be really efficient.

A

With this as well and as reviewers, we can do that by just taking a bias for action and pinging reviewers on an MR which um happens a lot in community contributions, but sometimes happens within Mrs within our own team foreign.

A

This is a good question. um I do think. uh I do want to go back to uh your question. um I know, I, keep saying your name wrong, because I forgot the actual pronunciation of it, but I keep saying Casio but I I, remember, hearing I, remember knowing that! That's not the right pronunciation is that that's not the right pronunciation.

E

Almost uh it's Kasha.

A

Thank you, I appreciate it. um uh You asked the question about one big Mr and earlier Yannick was talking about splitting up Mrs and splitting up within parts and I want to rant about something as a reviewer. This is my least favorite thing that I see happen is.

A

I usually see it this way. This is the story that plays out someone's, somewhat somewhat green, to contributing to gitlab. Their Mr is a little large and they get feedback. Hey your Mr is a little large. So then they start like aggressively horizontally slicing DMR, where it's not just here's the back end, then here's the front end it's from the front and it's like here's, the component I'm going to use in a future Mr and that component's not referenced anywhere.

A

uh That I've always had a hard time reviewing because as a reviewer, it's so helpful when you're introducing new units or making changes to see. How was this new thing going to be used and consumed so the more as reviewers- and this happens as well of talked about like coaching efficiency by example.

A

Oftentimes, you may find ways to just coach: hey here's a different way. We could have sliced this up. It's fine now, but if we sliced it vertically, that's my favorite way to slice things up if let's integrate everything very simple way, but then we'll expand these in further parts that even allows like not just part. One part two part three of Mars that are stacked on top of each other, but you can even have after you have it all vertically integrated.

A

You could then have multiple Parts all pointing to to master, which is a highly efficient way to split up your Mr, um but it does take a bit of creativity and a different perspective, sometimes on how your um framing it, and it is sometimes tough to keep it all reviewable. So it's not clearly.

A

This is just a desired outcome, but it it's there's situations that might not be appropriate, but um I'd love to hear your all thoughts on seeing Mrs that were split too finely and the kind of feedback, and if you gave feedback and and how do you handle those.

B

um I'm just sorry.

D

B

Really to add for myself what I'm just looking for is, if I recall correctly, it's actually in one of our iterations sub values to split those things in a vertical way, rather than horizontal I'm looking forward, and that's how you go ahead or come back to this. Oh.

D

It's just basically what's going to validate what Paul said if I don't see this as a front-end, maintainer I, don't see how this is implemented on a real page, it's not a feature I'm sorry. This is not a minimal, viable change. If you just added a component that is not used, nowhere at least put it somewhere on the page, so I could test it properly.

D

Without doing it myself on checking out the branch you're like okay, where do I put this component to test it so yeah and I've seen smrs I have no idea why people do this, maybe in the past it was for mr8, but now it's like it's very hard to review it's very hard to follow, especially if you're not a single reviewer or maintainer.

D

It's like it's a fifth and more in a row, and you have no idea what's going on here. Everyone has seen this like chain of the Mars when it's not five but like 10. and you're. The seventh.

C

I have not gotten anything with that. That would be really confusing.

D

um I had it multiple times as a maintainer, it's really really confused.

C

um I really don't know if you could get a back end split up into like 10.

C

I I, unless you have like multiple database Mrs, that you've split up and then add a model and then add this and then add you know and maybe like add a.

A

Circle right and then add.

D

A

You ever cmrs like that.

C

A

Okay, there you go no.

C

I've had one where, like there was, the database change made where, uh like an index, was being added, but then I couldn't see it used, like I, couldn't see what they were changing. Maybe it's like a scope being added like an index being added to support some scope and then I did have. There was one Mr where they were like. Oh, this is how it's going to be used, but it's when you're reviewing the database Mr.

C

You need to look at like the performance of what's being done, and so that's the only thing I've seen that was somewhat similar, because if you don't see how it's going to be used, then yeah, it's definitely like a balance because I don't want a large Mr, but also um it's hard to give the correct feedback on it. When there's no way to test it.

E

But I guess and.

C

The database wants to make me nervous. They make you nervous.

A

No sorry I was going to say that I received non-verbal communication from you Terry. Every time database comes up like you're you're, like scratching your arms or you're like feeling sweaty, I'm, joking I'm, sorry, Terry I, don't mean.

C

To yeah, it's like it's, it's it's right up there with the a CSS man. It's right up. There.

A

That's funny uh I have a question for you. John like, but first I want to hear what Kasha, what were you gonna say.

E

uh I was just going to say something about the backend side of things, so um I can see someone working towards like a class or a filter, or something like that uh that they, they not sure like it's, not fully baked, but it's kind of Half Baked, and they just want to build on top of that and and they can merge it.

E

It's also controversial because it's not used, and so like uh someone might say, oh because it's not used like you can't imagine, but because some pieces of work are so large that you kind of you can't apply like have it built and then apply it because then we will have to put it under a feature, flag, etc, etc. So I think there are cases when we do decide to merge something that is not used, but then, like next steps might must come straight after and it can be like.

E

Oh no I'm, gonna pick this up like in two Milestones I would probably not work.

A

Oh a hundred percent, especially since we have uh you, know, Untold thousands of contributors and hundreds of commits a day uh this this you can't ever treat like this is my code and I'm gonna work on it later. It's like now, once it's merged it's out there and um many times, we've received Community contributions on code that were slated to be deleted, uh and there was no obvious sign that oh this code is someone else, is working on removing all this, and so things like that is something to be aware of yeah.

A

Thanks for bringing that up, uh question I have for you got it. I think you hopped on the um the interview, training and I was going to ask. Do our interview Engineers? Are they trained on like picking up non-verbal signals like interrogators to know if someone is like if someone's lying or I don't know, I'm joking um they'll probably be not great, um because not.

B

So much there's there's like a prototype every morning to their heart rate during the interview, but that's about it. um Just I see something.

C

A

C

Even look at the person, oh when I'm interviewing I, honestly, not even looking at the person. That's probably.

A

C

uh Yeah I I well at least it's like you're supposed to be really kind of like taking notes. I I find it like a lot. It's a lot of like almost like contact. Switching where you're trying to like take notes, digest what they're saying help them through the process, not freak them out too much. You know, I I, don't know, I feel like I've noticed. A lot of people seem nervous in interviews, but yeah I, don't like them. So.

A

C

Don't like it you're interviewing or you don't.

A

Like interviews, I.

C

Don't like being in an interview on either side but.

A

C

Yeah, it's I feel, like both sides are pretty stressful um but yeah. It's interesting where you see if someone uh is like really but like when they're nervous, it's like it's an interview and it's.

A

Scary I know I'm I'm such a bad I'm, really bad at interviewing, because I like really want to believe in everybody and I, want to make everyone feel better and I I'm the worst, because I'm I'm such a you know like Ed I've done like software education, I've done and like that's, where I'm at so it's hard for me to put that aside and to just I'm evaluating you now as that's really difficult for me to do.

A

um Yeah uh well lots of good conversation. We have a few more minutes to hop on reviewing um an MR. If someone has an MR that they would uh think would be interesting to uh to offer upon the mob.

A

I have some Memoirs on my DOT on my plate. Italian knows I have one Mr.

D

Yes, don't don't bring my alarm, it.

A

Felt so bad I um didn't get to get to it and I felt I went to sleep feeling bad I'm, really sorry, but then I woke up this morning, I was like Natalia's, nice she's, not gonna yell at me, so laughs.

D

I mean that's the Mr we already had on the fountain pairing once right. It's the same thing it's front and cash in very problematic area that we have.

A

D

Just I'm not happy with this smart either: okay, I! Don't like the concept! I, don't like working on this concept! I, don't like the fact that we will merge it in the code base. So that's fine like it sometimes when, when you speak about it and front-end pairing on maybe on this session, like you need to defend the Mr, because it's your work but I don't want to because I don't want frontal caching to be in our code base. That's.

A

Funny I do um one of the comments I hadn't, yet written both in my head was like. We need to establish a guideline in the front end and like a link to it of. When do we do this, and when do we not and let's lean towards always not like all of that would be great to put in our front-end guidelines.

A

um That's a really interesting Mr. um I really appreciate, like that's a great example of you. Pinged me really early of just like hey here's, the draft it works and I'm leaving comments which You're Expecting, because it's here's and I know like hey here's just the draft of it, so um that that was a cool Lamar. That was not the Mr I was gonna bring up, uh but we could and maybe just get it done with um yeah I uh I know I have some um I think smaller like front end.

A

Mrs um there's one I was in the middle of uh had some interesting ux questions which I kind of resolved in internally by just asking a question, um so we can hop onto that one. um It probably wouldn't take that long. um Let's go ahead and do that then. uh Let me find my my zoom controls.

A

Does anybody notice, how do you do that I, don't.

B

Know what we can say out of Korea something I'll tell you the front end. Caching Mr. How large is the diff, because I imagine it to be massive.

D

I think it's around 600 lines: oh okay, I.

A

Imagine my first okay sweet good and it's only like two months, there's only like two modules that are the heavy lifters. Everything else is kind of like decorating. It was like. I was impressed with the effectiveness with how small of a footprint it was like I was I was I was impressed with that um uh all right, I I noticed um we have uh a new I, have not interacted with this front Ender before, but um she has a lovely name. uh I don't mean that in a weird way, but her name is Paulina.

A

uh All of my friends in high school would would call me Paulina because they were not nice uh foreign.

A

So this is one I was reviewing locally and the scope is this.

A

We um previously on these uh timeline events.

A

We uh did not have this event tag, so we have the ability to tag events.

A

um And currently I don't have my GDK running I'm not going to try to run it right now, because that'll probably go south the fact that we're on a club um currently we only have two tags and you can apply multiple of these tags, but we have start time and end time so that that sits as a little strange.

A

But the author does um comment that uh these are the only tags and that they're contradictory, but the data model is that we're going to have multiple of these texts so moving forward to something like that from the user perspective. I was really wrestling with this, because I was like, but that's so weird that I can select. This is both the start time and end time, but uh this Mr was also ux approved.

A

So at this point I'm kind of like okay I guess he had a pick my battles to some extent of what I think is uh weird and not weird.

A

um So it sounds like this was some sort of ux and product decision that we're okay having start time and end time, be our only tags we can select, but other than that. um The other comment I left was this. uh Let me go to the oh, no I have my review comments here.

A

um Yes, one thing that stuck out to me as strange was we're only showing this. If we're not editing, so we only show it when we are creating a new event, but the edit form hides this, and, as a user like I, would imagine being super confused. If I was trying out this new input created it and it wasn't able to change it, I could see that being a confusing situation.

A

um She highlighted that, but I went back to the original issue and so I'll know the goal. Is that we're showing this in both the create and the edit, so that um it's sticking out to me as a little strange, so I just ask a question and usually I find myself timing myself and if I'm can't find an answer to something within us time. I need to start thinking about how do I frame a question here.

A

uh The other comment I left was um yeah. This is an interesting one too, from the front-end perspective, when you click on these tags, since I don't have the GDK run, I can't show you, but when we click on these tags, um it'll populate uh the Box, the text box, with the description with default based on the tags you clicked, but because we have this condition of hey. If we're empty, then populate it based on our tags being changed.

A

That means after I've done the first one it'll never will update again, uh and so the normal pattern I see that we do for this is like, rather than just testing for emptiness we test for dirtiness. We have to kind of insert a new state of like. Has the user ever touched this once the user touches it?

A

We don't ever try to update it again, but if users never touched it, then we'll uh then we'll um uh be free to update this, uh and so this is a pattern we do a number of times, I've seen in a number of reviews, so uh I was able to kind of um touch on that. The interesting thing to take away here is this Mrs ux approved, but- and this is clearly a uxc issue.

A

But if there's a low hanging fruit that this might be an oversight from ux, we all kind of need to own the end user experience, especially as reviewers and maintainers, by asking questions and making suggestions like this, uh so I can see how one person would just take away. Well, no I! Guess it's just how it's supposed to be because it was ux approved, um but we don't really get the benefit of having multiple eyes on it if we're not willing to all contribute to the discussion.

A

So uh that's why uh I jump in here on things like this um great, so my other main task was just doing a one silver on this component again and then looking at the tests, and then that was going to be. That was gonna, be it um so let me I had the changes checked out here. uh This is me making that patch. So let me just revert what I was doing here.

A

So for this, oh, my gosh, all right, y'all y'all, faces take up a lot of screen space.

A

All right so as I look at this component, we are adding a previous tags.

A

Which we initialize here in the data I, am sensing uh correct me if I'm wrong, Natalia uh we'll need to like copy this array. If we ever, if we actually use previous tags, can you put props directly that'll, be.

D

Right, it would be much better to do it at least a shallow clone here, instead of just doing that, we can. Of course we can't do this assignment, but I'm just doing a shallow clone, at least shallow clone just for the sake of not referencing the prop okay.

A

Yeah, uh this um somewhat exposes the prop to internal uh to as internal state, which can be mutated, and since we can't mutate props, um you should.

A

Consider shallow clonings uh like so right, that's JavaScript, right! I! Do.

D

A

A

um Cool uh and I guess that's the only place previous tags is used yep. Where is this tags just tags use? Let me see if I can come up with fancy project for me to find.

A

Oh, it's used here as our items that we would select. Okay.

A

um There was a comment on the Mr relating to this as well, where currently we just have these two hard coded, but it looks like they're coming from other places eventually. uh So that's fine.

A

This is our condition for all right, so let me keep going down so that looks fine, our condition for re-dropping down text, which is used in the drop down we've only selected one. Then we just use that value, otherwise we're printing out how many tags we have.

A

What, if we're not selecting any oh, but then we go here to then figure out if we're not selecting any.

A

um This is a minor thing. uh I would probably do it this way.

A

I, don't know what do you I'm not trying to be nitpicky but I'm? Just these are just small suggestions. This is a non-blocking suggestion. I, don't super yeah because then I.

B

Can just can I just really quickly sort of back to the suggestion you just left like shallow clone would be the exact same thing as Shadow copy correct. It's basically creating a new object with the exact same.

D

Yes, yes, exactly I think I have a suspicion that you might be doing it somewhere on the background, but it's better just to be saved than sorry. So whenever I have object or array and I put it from props to data I do just a shallow copy.

A

Yeah, that's that's a really interesting point about the receiving complex objects as props the need to do that. um Man. That's tough.

D

Yeah and resigning them to data is another problem, because, most likely you will need a watcher on previous tags to make sure that do you need to update the current exit. Previous tags are reactive or not, but that's a different story. Sorry, okay,.

A

I, don't know which, which is, would you classify this suggestion that I'm making here as a non-blocking suggestion or just some minors to test.

D

Nick pick this.

A

Is a nitpick, oh man, all right, this guy even promoted to nitpick all right, um uh ending this with a ternary might be a confusing or it might be less readable than an early return. What do you think of this.

A

B

Right, uh what I'm laughing about is um I totally get I, get it and um like let's put opinionated, and it picks in these Mr I'm laughing, because, like just this morning, I gotta sit on an Olympic, but it was exactly the other way around. Like I returned early and had the uh where you were asking me to return internally. So probably something going on not.

A

To return early and to use attorney yes, oh wow, wow!

A

This is my Mr, so we're gonna. Do it live I'm just.

A

uh Okay, cool yeah and then this these all props make sense. So then it was just looking at the tests and um I showed you. My cool coverage thing I'm playing with so I'm gonna see if that helps at all, um I really wanted to get the table spinning out, but thanks for helping me uh get that one to where I'm about to submit it now so appreciate, everybody's hopping on and hanging out, you all have a great rest of the day and I'll see you later bye.