GitLab Frontend group, 8 Apr 2020

Previous Meeting Next Meeting

⏯

youtube image

►

From YouTube: Frontend Maintainer Review Shadowing

Description

MR being reviewed: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/28173
Pinning Tests docs MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/28330

A

Okay time for another maintainer shadowing session today, I'm going to be reviewing what I've been warned is a doozy of an mr. So please make yourselves comfortable. I have no idea what to expect. Then what a doozy means in this case, but I understand it's a serious refactoring. So I'm gonna share my screen.

A

Can you see the mr great okay so, as usual, just to get my bearings I have a quick skim of this refat to compare version dropdowns, so I guess that's the compare page when we compare do a diff, basically using git lab github, see why I can imagine that that might be fairly painful and just to get a quick look, 17 files of change. It's actually not too big, 6, 4, 4, +, 6, 4, 4, 2, 4 8, so I'm not actually too concerned about that.

A

However, there might be pretty serious data structures that I don't understand, I'm not familiar with so we'll just have to take it one step at a time, because this is potentially quite difficult, so complex, I'm, gonna, read the description then read through the comments that have been left already. Not that there's not a fare for you, so I'll try and go through this as best I can.

A

So for a bit of context, I might read this in a sec. This is a refactor of the comic version dropdowns when displaying when, when developing displaying the head.

B

A

A

A minimal merge, ref based different emerged. Okay, I, don't know what that is. Quick. Look at that.

A

Until changes from the target branch are merged into the source branch, creating a complete mess of the death.

A

That's surprising I would have thought this would always give a good death, but maybe not.

A

Okay- and this is the initial version- I think that was attempted.

A

Go away: okay,.

A

C

A

I, don't really want to dig into that; it's maybe that become clearer later on.

A

Let's give a quick the main issue itself.

A

Comment on my directives.

A

Okay, a very similar description to this one.

A

Okay, so I guess you know whether, when a motor quest has merged, you get a merge commit and we want to be able to they want. This issue is about adding the ability to comment on that. Merge, merge cop, commit.

C

Is that right am.

A

I understanding this correctly I'm, not too sure so.

D

A

Going to dig in to the actual Emma there are some open threats, so I'll start with those first I guess so it was there anything else in the description.

A

Original snapshot that had some grouse in it, okay,.

A

Okay, okay, okay: this is talking about Jess snapshot. I, see for the record. This ugly death happens when you do a snapshot on a rappers HTML content rather than the rapper's element. If you do a rap diff snapshot element, you don't get this. You get a nicely formatted prettified.

A

C

A

Okay, so I don't know much. You understand that was going on there. Some pipeline failures.

A

His head is already broken, must at the end of this, MRI is a precursor and art fixing his head. So presumably this is a refactoring so that his head can then be fixed later on my have his head in the layout there's ready when the logic squared away, but we are not fixing the logic in this and let's try to keep this really simple as possible. Okay, so that's interesting. That's worth knowing.

A

That's a first I've, never seen a little rhyme in a.

A

Just out of interest I, sometimes open up her a resulted threat just to see what was said great job, resolving a template here: okay, I.

A

Do this sometimes to help me understand the context, because Paul obviously wondered about this and I might end up wondering about the same thing. So I might get an answer before I get to see the code.

A

Interesting, funnily enough, so the question here: why are we using amount rather than shallow amounts, I, guess and then declaring some stubs I have experienced occasions where shallow mount is not what I want, because I want to verify that an entire subtree renders correctly, which is not often the case that sometimes that's what you need to do, particularly if you have a functional component, because you can't do anything you view testing tools doesn't work well, when you shall announce that there are functional components in there, you can't ascertain anything about them, which is kind of useless interesting, in which case Melanson, in which case mounting, is the thing to do.

A

And then you stop everything except that component, and that means that everything under that component tree is also not stubbed. So sorry carrying on getting digressed.

A

So this is actually an open thread.

A

Okay, so this could have been used in showing up.

A

Okay, I, don't know what's going on there, so then we're.

B

A

Fine Paul seems happy with it, so.

A

There's a pinning test involved in case you know we're pitting test is wearing you assert.

A

The complete world, for instance, where you assert a complete rendered outfit and is put in useful during Ares refactoring, where you don't expect the rendered alpha to change, but your refactoring, the implementation. So you pin the before state, do your in factor and always refer back to the pin and make sure that you're not changing the rendered output, but the actual pin itself the sort of snapshot, if you like, is not kept. It's just a throwaway constructed out of the receptor.

A

Admittedly, I'm I'm sort of half reading these I'm kind of glossing over because I just don't know much about about this area. I think that's a sign that I need.

A

Good, mr of rhyme, pictures great, maybe that's why I said it was a doozy.

A

Yeah interesting, this is very much a middle man component like it's fitting entirely in this diff view. In this comment, comment death.

A

But I think there can be justification. So let's say maybe he explains it.

A

Okay, there you go doozy of enema time to dig in Quick Looks, we've changed a bunch of components, changing some stuff in the store, not a lot in the store. Really mostly, this new file.

A

Which I guess is it utils I know it's just guesses, okay and then I loaded tests so actually we'll see.

A

Okay, so we're mapping a load of state from the store and we're no longer mapping the target branch name.

A

Of the dips component now, what's Justin said about this: move this to a getter, okay. So what state? Now it's a getter, fine.

A

Okay, interesting so, basically in the parent app. What he's done is this target branch name was simply passed down through to this compare versions component, and this app seems do nothing else with this information, so he's extracted that into the getter and then is presumably consuming it directly in here. That's my guess, which is probably the case which didn't mistake me.

D

A

And now we have a renamed file from compare versions, drop down to compare drop down layout, okay,.

A

So he's removed a lot.

A

Which suggests that.

A

What's happened is that this component presumably had both dropdowns in it, as my guess, where's. The template.

A

Not especially.

A

No I'm trying to understand what this mr is trying to achieve.

A

Because the most valuable thing I think a reviewer can do is suggest a better architecture. Everything else is kind of secondary, so I'm trying to understand it. For that reason to see if I understand the architecture and see if this refactoring makes sense, I could ignore that and it would make the review go much quicker, because I could just focus on surface level. Things like are, we are we conforming to our coding conventions and so on, but that's less valuable. In my opinion,.

A

Sorry I didn't catch that.

A

Probably roughly felt follows that pattern: yeah I try and get an overview of what's being achieved, what's being done and then I drilled down into the details. The nitty gritty as I go as when I feel more comfortable sitters going into it. That said, you know if I see something immediately bad like I.

A

Had an example, but then I'll make a comment right there and then, but right now, if instance, I'm trying to understand what this refactoring is. So he's moved as far from here to here why the naming suggests that this used to contain a drop-down. But now it just is the layout for the drop-down.

A

So, let's look at these new files actually compare drop-down source. These are the middle known components that came up in that common. Actually,.

A

Okay, so the naming of this actually threw me, which is interesting, I thought this would be a drop-down, but you know there's nothing. This doesn't imply a drop down to me at all.

A

What I expected to see here was maybe this layout component, but then as a maybe a default slot was the actual drop down. That's what I thought I'd see, but that's not what I'm seeing so I don't fully understand what he's trying to achieve here.

D

A

Which makes sense and they are laid out by the layout yep but I'm- not seeing that yet so, let's, let's go back to the layout and see what's happened so.

A

Actually, just want to go back to the app because compare versions drop down where was that actually consumed.

A

Is that this compare versions, I think it is.

A

A

I see, compare versions consumes the drop-down swords drop-down target and the drugged-out source and target consume the layout interesting. Okay,.

A

So one thing I am aware that I need to understand here is that there's a lot of props that have changed a lot of props haven't been removed and refactored moves read from one component to another. Imagine like target branch well, I. Think target branch now is a getter I. Think I, remember, seeing but like magic. West is based a suburban path where these are coming from I'm, not sure.

A

Yes, okay, so this component.

A

This component got renamed to the layout. That's when the bulk of it is that the source and the target ones, sort of manifestations- and that's I, just want to see what the difference between these it is.

A

So really I think I'm guessing the only difference actually is which getter we bind to here: difficult drop-down source versions and a target, compare drop-down target versions. Yet so I think that's the only difference here.

A

Which is interesting, so I'm going to look at the layout, because that's where everything happens next.

A

D

It's not actually so.

A

This this layout is a dumb component. It's not connected to the store, which is what I'd expect it's simply receiving the versions.

A

In a lot of computer props are just gone here: I, don't know, I, don't know where they'd all gone yet.

A

Okay, so yeah. The layout is.

A

Yeah I'd be more in trying to say this should be cool to compare, compare drop down, it's it's, the drop down, meaning I would I'm not sure layout is the right name for it, but we'll come to that later.

A

So basically, this has been generalized so that it's iterating over the versions that it receives and it, whereas you used to be just for target versions.

A

I'll go back here. How is this done before? Compare versions drop down so I? Imagine that they reuse the compare versions drop down twice in this template yeah exactly and now, they're called calling them something different.

A

Okay, so one way to look at it is this. This is essentially the smart container for the compare drop-down layouts, dumb component.

A

As if this is connected, these are connected store, but the layout isn't this sort of just abstracts that way.

A

I'm thinking about the props that the old implementation received here so the same component had other versions. Motor quest versions show commit count, but then the same component has other versions based version parts, start version and time crunch, which is weird.

A

So that's probably partly what motivated this MRR is that you know you're, giving a completely different set of props to the same component and it's kind of hard to see why so in that sense, I like this. This gives a name to what this is.

A

Admittedly, we have slots here that do something similar, but still it encapsulates little encapsulate sit a little bit better.

A

There's still a lot of prop movements and changes that I haven't absorbed. Yet, like all of this, this is the layout I. Don't know where that's gone is active. Where was that used.

A

Okay, so is active is gone and they just.

A

It's just the actual version, I see what it is active, D.

A

Ok, so, presumably now this is part of the getter which transforms the versions and prepares this information and as the selected property on on to a version object. It's my guess. I'd have a quick, look and see what happens then. So what? What are we? Looking for? Diff, compare source versions.

A

Yeah source versions yeah, so there isn't selected that I thought it's gonna, be there fine.

A

Also note there are 19 commits in this, mr, so it's been on quite a journey.

A

It would be interesting actually to look at those look through those comments, because I think there's going to be something interesting. Hopefully it's there. Well, if the commit messages are good, it'll tell a story.

A

Right, I'm, sick of looking at you single file components um we're going to start looking through the store and started state.

A

Of the ordering of this is weird, maybe I, say.

A

Where is that used.

A

Okay, I don't really understand what that is for yet I.

A

Don't I also don't understand where this is being consumed from gets versions.

A

Alligators are just react: sporting everything from there. Okay, I.

A

Quite like you, it sort of splits out the Gators. These are the Gators just for the version drop downs, I quite like that.

A

Anyway, so let's carry on so it's reacting from their fine.

A

Selected target index: where is that used? What is that.

A

It's not actually used in any template. It is used in other Gators. Okay, oh and that's how we determine whether it's selected, whether it's the active one and coming back to the layout.

A

That is used to determine basically I think whether it's the selected drop-down item. That's it. Okay,.

A

All right, so this is just a name for -1, which is the usual sort of index not found index. Thank you, correct, I, think so. Yeah.

A

So it's not immediately obvious to me why this one has a fallback, but this one doesn't quit. Looking to that other.

D

That's the state.

A

D

B

A

Thought this would be a good, mr to review, actually I'm, not so sure, because mostly I'm just trying to understand a bit quiet. Quite a lot.

A

Again, also just to reiterate that this is going slower than it probably would normally, because I mean I'm explaining what I'm doing and why I'm doing it and maybe I'm going into a little bit more detail than I would otherwise I'm trying to be more thorough here then I would otherwise.

A

This is an interesting problem. I like to see, what's going on, say.

A

Actually one thing I've I sometimes do is I write a little note to myself.

A

So I'll come back to that later. Basically,.

A

And this is just my own understanding. That's what's going on.

A

D

A

Exists if the user selected a version other than base so start version is northern base must be selected. Okay,.

A

I mean where is star vision, that.

B

A

So at this point, I just want to.

C

Have a quick look at that.

A

Mostly looking for start version- okay, that's not actually that.

A

Start version is never actually mutaters in the mutations, which is confusing I.

A

Don't know why I'll come back to that.

A

It's a dish compared to thousands.

C

A

So I'm wondering this list of versions, it's getting everything except the first merge question formatting yet and then appending the base version last. What was the previous? You know? How did this look previously? I, don't understand that I am I, think that that was mostly done in the compare versions. The old compare versions drop down, maybe.

A

All right because target.

B

D

It's basically this my thing.

A

Yeah I think that's what that is. Okay,.

D

A

A

Yeah, okay, so this is. This is exactly what I was just looking at in particular target: there's some pretty wonky manipulation to get base at the end of the list and hard codes and things into the base item.

A

So that's exactly what I was just looking at, which is interesting. Just.

A

Out of interest, how does the source versions compare it's doing a plain map and it's producing a very similar structure, so href version name selected? The difference is that source versions, for some reason has a commits text.

A

Where is that used.

A

So that was a method in the drop-down before, but is now actually part of the.

A

The let that drop down the layout, so this was a drop down item I, guess.

A

Show commit count. What was the previous rule? There.

A

There's actually a past property, okay, Wow right.

A

Okay right, that's that okay, so this is now being encoded directly into the versions getter, as in the source, has the commits text, and that is the equivalent to this thang.

A

So my developing understanding of this is that what the the the substance of this refactoring is taking a load of component computer properties and methods and so on. All of this stuff and encoding that directly into the version objects that iterated over and rendered.

A

Which actually makes sense to me I think that's a cleaner way to do it, because it's it makes the templates simpler, as evidenced by minus twelve, an app dot view 31 on compare versions and these tiny middleman components. I guess the this diff. This number actually is not accurate because we're not adding 78 lines here, which is really weird.

A

Why is it doing that.

A

We definitely deleted more than we added same.

A

Continuing with the getters, so okay, so this is encoding the difference, differences that was present in this.

A

Guess exactly so versions are. You know I always like to think of this now as using typescript initiation, so we now have a version which is something like.

A

Href straying- and you know, and a bunch of basically these fields where comments text is optional, which is a string and so on lots of other stuff as well. So it's created.

A

It created this data structure basically and that's the rising version.

A

All right so here we have those two, those two Gators fine, so they're, basically just making those arrays of versions that we're passing to the drop downs to render, which makes total sense.

A

No, unless target of selective comparison, there's not based. If, yes, that's the same comment as elsewhere, which makes sense what was what was he writing their future me already appreciate settings coming here, great.

A

So I'm now wondering so I'm sure this. This, mr, is big enough. I, don't want to suggest a different refactoring on top of this, however, that this comment is necessary suggests to me that this data structure is not optimal. Maybe there's a different way to encode this in the states. That would not require this comment so I'm going to keep that in mind, how's it going. ah It needs to look at this snap right now. Oh, it's really ugly as well so I'm gonna.

D

A

He's done a wrapper dot HTML there it is yeah, so no he should be doing rapid element to match that job and he'll get a much prettier diff.

A

A

I could point to this as well, but I'm hoping that that's clear.

D

This is actually.

A

A pinning test, okay, well yeah, because because this is a pinning test, maybe it doesn't matter that it's ugly, it just matters that it's the same still I I. Would you know it'd be nicer if this was easy to read and if you do have a difference, if it's prettified, it's easier to see what the difference is and- and maybe yes.

A

A

Okay, app spec.

A

So he removed the target branch and the merge quest. Diff props from the pair versions.

A

Well, he didn't, which is interesting.

A

So that's already something that I think is not quite right.

A

I have a feeling this might become clearer as I go on, because maybe this is a well I, don't know. Maybe the data structure here changed or something but I'm, not seeing that so I don't know.

A

Let me just click to the line, don't know which one of these essentially adds the same one: okay,.

A

My conscience look that way.

A

But he did definitely remove the target branch, which makes sense, there's a bunch of other crops, so he's only actually testing one crop here. That has passed compare versions, but we are passing I'm just going to change this back to the diffie-hellman.

A

It's not clear to me why we're not testing that this was passed through. This was passed through. This was passed through. It's a refactoring, though so I don't I'm, not actually too bothered don't care too much fine.

A

That was nice, simple change, so he's added this spec I, don't quite want to read through that. Yet so the drop down the source spec. This is that middleman component. So you know the test is way bigger than the actual component I mean all it needs to assert really is that it passes the correct gets a value down to the layout, which.

A

Yeah, which is what he's doing.

A

Okay, so these two I'm guessing correspond to these two I.

A

Don't know why I don't think this Stubbs is necessary when you're shallow melting, I, don't know what this is achieved.

A

You shouldn't have to specify this anyway. The only thing I I'm, aware of is that this might change the stubbed name of the component if you're doing a snapshot test, maybe this so I'm asking basically because I don't know is maybe there's a good reason for this I'm. Just not aware.

A

C

A

So, just to come back I feel like this is so he is actually using the actual module itself. So this is less of a component test and it's sort of an integration test, because it's verifying that the diffs module is mutating. This in the expected way, I.

A

Was expecting that this suite would just verify that it that component passes through the correct getter to the drop-down layout, but it's actually doing a bit more than that. It's it's verifying that the diffs module is mutating the these correctly, which is fine, sometimes that that adds a lot of confidence in. What's going on so I'm happy with that.

A

So the target is going to be doing something, presumably very similar, I.

A

Mean this is all just boiled plate so far again.

A

Another thing to note is that my comments are normally not so terse, but for the sake of trying to keep this cool under three hours, long.

D

D

A

Feel free to drop off any point.

A

Bass version has moved to the end of the versions list. Yes, we looked at that earlier, which is why version 2 is. The first version is. The first is the first.

A

In the versions prop of compare drop-down layout.

A

Yeah, okay, that makes sense version. 2 is not just the second one, but that's the base. One I guess is what he's saying: no, that's a big one: hmm okay, I'm confused by this version index and.

B

A

Version 2 version 2.

A

But is actually our version is that one yeah? Okay, all right I'm, confusing myself as what's going on.

A

By again so this is very similar to the other test. It's a bit of an integration test with the diffs module, which is fine.

A

Compare versions drop-down spec deleted, so this.

A

So this is a case where the get lab.

A

Interface is showing this as a deletion and a new test. It's not recognizing a move.

A

This is where some get magic comes in. So actually let me Graham.

A

Let me grab the branch name online and.

A

A

Okay, so what I can do do a diff of origin, master against origin and the branch name, and let's just stop first, to see how big so six, four, four four ten that's already different to this. Actually I can tell you why that is. Oh, it's interesting! It's really different! It's the same number of insertions, but more deletions I have.

A

The algorithm configured to histogram, which is not the default, the default is Maya I think is too, and this possibly a better algorithm. So that's partially one difference, but you can tell diff get diff to detect moves better.

A

That's not produced difference because we're still seeing there's no moves here. We can tell it to forcibly attempt moves which I learned about this option the other day. Just it's still not detects the move. Okay, which means maybe then that this compare versions, drop down. Spec is really just completely different. There's no similarity whatsoever between this one and this earth, which is maybe not too surprising, and if we actually look at the files, maybe I'll I was hoping to teach something new there, but it didn't work out, never mind.

A

Job done spec, so this started by saying select version name shows the latest version regulators to select it compared to.

A

Yeah, okay, all right, I know I'm, not looking the right thing. Sorry, this one.

A

Yeah, that's already looking very different, so there's probably not of oh. In fact, it's tiny!

A

That's why it's tiny.

A

What is this test actually asserting.

A

B

It doing a full mount.

A

Staying a shallow man.

A

What are these billions mean? I have no idea, the first say the second third and fourth arguments his base is head selected. Okay,.

A

So not base his head is selected.

A

What's the default for that, it doesn't matter so, presumably.

B

A

A

A

D

Interesting this.

A

Is kind of like a snapshot test, I guess but he's extracting the data algorithmically programmatically, rather than just doing it different. The output, which is fine.

A

Okay, yeah I, see he said, he's extracting things like this, which might otherwise be a bit tricky. Yes, I created a test created out to find out to find yeah.

C

D

A

I, quite like it out of interest, how did the old one look.

A

This is a weird one: I don't understand.

A

B

Other versions, plus.

A

Bass, the bass branch I.

D

A

I guess yeah. The point is: that's now been extracted out of the layout, which is why that test no longer exists, which is fair enough.

A

So do we test the href, we do don't we yeah the href leaving is active. I.

A

Think text already trims it, but whatever it's fine.

A

Okay, I want to look at the implementation one more time of the layout just to get a sense of. Is this really as as simple as just listing under versions, it kind of? Is it's just a very deep HTML structure which just it doesn't seem to have any or many yeah icon in time ago? Very simple. So this is pretty pretty plain. It's right for a snapshot test to be honest, but what he's done is fine. It's equivalent I think.

A

Did he test the comments yeah? So his head is bass, comments text so he's checked those variations.

A

Which seems reasonable to me.

A

Yeah Testament text.

C

A

Okay, cool, okay, moving on really looked at this one.

A

Compare drop-down layout.

A

Sorry yeah we've already looks about one.

A

That's a suggestion. We've kind of looked at this.

C

A

Yeah, so all of these are implicitly in the new spec, which is fine, minimize that oh and then now we get on to the rendering bug that, what's going on, oh.

D

A

What's going on.

D

A

Have no idea that was really weird.

A

A

Okay, so this is testing the new getters. This is kind of where the meat of this is because it's all about this whole MRI is about transforming the props into the you know. Making the data structures actually represent, what the props doing for.

A

So I there was a discussion early in this, mr about his head, is broken. I still have no idea what that means that hasn't come up at all. As far as I know and I don't understand what is broken about it. I mean I. Think there was something.

A

It was this disclosed one. That's right.

A

D

I stop sharing.

A

I have I, don't know why.

A

A

Head was applied to every version, I mean.

A

Okay, very similar sort of settled bars have changed here. So this is the aborted attempt.

A

So I have lost bearings yeah.

A

Yes, I, don't understand what the is head being. Broken means I might go through the comments again since try understand.

A

Okay, so I was a bit confused. There.

A

Okay, a pattern I've seen elsewhere is that you surround you just so. You describe comparative version, drop-down getters, then you have a nest of describe called this, and then you have your assertions inside there.

A

So this would be a sub describe for select target index and this would be a describe for selected source index, which is why has a bit confused? Then I might suggest that, because it makes things a bit clearer.

A

So we'd have the top level described.

A

Which is sort of what's going on here, but he's doing it with a top top level single bit, which is fine as well, except when you have two tests for a given one. It makes it a bit less clear. What's going on in my.

D

A

I have sorry I have a.

A

That because I'm, a Mustang here, I.

A

Have a context activate, so it does something very similar. So as I'm scrolling down, it's actually not very good nested, but you can see the the parent context is described there, which is quite a limb, is great. You should all get into them.

D

A

Then what happened? What happened.

D

A

Have never used Emacs, no I might I think I'd get RSI from all the corded.

A

A

If compare drop-down, so diff slice.

B

A

Array at the first position: yes and appends, the base version yeah. Okay, what I don't understand actually is why we dismiss the first thing: I.

D

A

Understand that I mean presumably that's what we were doing before I would.

C

Have been here.

A

So, are you other.

D

A

Let's go quickly into that. Compare versions, drop down.

A

Other versions was used. Well, it wasn't doing it here. As far as I can tell unless other versions Oh.

A

Getting weird rendering again I think that's a Firefox bug.

A

B

That's just coming.

A

Straight from the state.

A

ah And a he go slice slice one. So that was something that was done in the first place. I have no idea why what's.

B

A

Merge request if I don't know, but they don't want it so fair enough and that's been done consistently. So that's fine.

D

A

I'm tempted to ask.

A

Yeah find out D.

A

So this is the expected shape of what so the first target version needs. This expected state shape oh yeah, because we talked about that typescript shape and he's adding these things so.

C

A

To contain equal target branch.

A

Waste hiding French what is target French.

A

I'll, learn that okay, this this is the base brunch, okay, based version.

C

A

I should say.

A

And it contains that, so my understanding, in fact, is that the last item in target versions should be that so I might even and I might suggest that.

A

Because that seems like an important part of this of the whole structure is that the base branch is shown last.

A

An SMS understood.

D

A

Yeah fine and this final one again does the expected shape false, false.

A

There's not a Tet, so there's not a test that verifies selected is results in true, which I think maybe is quite important, because.

A

The actual get is, if I find it. ah Branching are changing selected based on certain things, so particularly this one, this one which relies on another getter, so I think it's important that this is tested. So I'm gonna I'm, going to suggest that.

D

A

Is- and this is this is where it gets kind of messy right with integration.

D

Yes, because you.

A

Don't know what is tested where I.

A

Mean you know they've come so well. No! No! ah Where is that integration test? Can you remind me.

D

A

That the source and target tests- wasn't it yeah I think so.

A

A

No, it was the.

A

It was the layout spec, okay,.

A

Right, yeah, yeah I know it's so it's being tested that so this.

D

Is a version and.

A

We're asserting we're just setting the.

A

Selected property to true here, we're not verifying that it can be selected to true, can be set to true in the first place as far as I can tell yeah. So this integration test doesn't cover that.

A

Okay, I bleach from because I can't type shroo, shroo, yeah, apparently, okay, so I've I've read through all of it. I, don't fully understand the sort of base the core data structures. I still don't understand why his head is broken. What's broken about that, I've still got rendering problems. What's going on.

A

But I think I understand what this mr is trying to achieve and I think the refactoring them makes sense.

A

Transforming data models is a much easier way.

A

You know, then, in this case I think it makes sense to transform the data models and then just render the plain data structures like we're doing in there in the layout, by just relying on version, dot, selected and stuff like that. Rather than doing all this nonsense here, it simplifies this component quite a bit as we can see from all the computed props that are removed and props loads of stuff. So I like this I think this is really good. Refactoring and.

A

I, mostly just have questions about my understanding, but on the whole I think this is pretty good.

A

There was that file compare.

D

A

Specs so, and it would be the bit which isn't rendered correctly, wouldn't be.

A

Why why is that not coming up anymore.

D

Guys the wrong one.

A

Make sure we delete this before merging I, see okay.

A

Okay, I'm gonna, it's time to look at the commits, see what the history was.

A

D

Can you say.

A

That again, do.

D

You have a check out, the branch run it locally and then just debug it that's really cold I.

D

A

And actually, to be honest, this is one case where I probably normally would yeah, because I want to see how this worked before and how it works. After presumably, they should be identical because it's just refactoring the reason I haven't is because I want to zoom call and showing my screen and running the GDK will will kill things. So it's not going to work yeah so normally actually and in this case I would but in general it depends it's if I feel like I will learn something from doing so then.

A

Yes, if I feel like there's, maybe some brittleness or edge cases that I want to verify or explore. Then, yes, more often than not I, don't if I'm honest takes time, it takes too much time. Yeah exactly you've got a you've got to draw the line somewhere. It's.

D

A

Of all, he had a depending test, which is quite a nice way to start.

A

He had it the snapshot, he wrote to the existing spec, he just wrote the snapshot out. I can't create grandparents Kristoff's.

A

Yes, this already prepared some data diff mocks. Fine. So presumably that's reasonable.

A

Have two commits.

A

Refactor compare drop downs with splitting fairness. Three separate components layout depend on to higher-order components as Grayson flow code and testing.

A

Okay, I know what that looks like roughly and back is active. Interesting I didn't realize that you would have lost that.

A

So we know that at some point this he they changed their minds and this became part of the geta.

A

So one thing in this case: what's really nice about pinning tests and especially when you preserve the commit history like this is in theory, I can check this out run this test, verify that it passes and then verify that for every commit it passes, and ideally that would be the case.

A

And in fact, now that I'm, looking at it, what I want to do is just look at the commit history. Where was it? Oh, it's actually not that king heading test added I want to look at the history of the snapshot that they created great, so it wasn't modified since it was first added which is great, which means that as long as it passes at the end of the commits commit, you know what I can do that now.

D

The first time I hear about that technique. It's.

A

A really good technique, yeah.

A

There's an ongoing mr to document that as a technique for refactoring.

B

A

Jeep, do you know I probably need to generate fixtures, for this? Is my bed yeah I know it actually passed. It's just super slow, wow.

B

This is so slow.

A

Great even got.

D

A timeout there.

D

A

Is the zinc wall and you know what I can I can just compare those in spec? That's that's weird.

A

Lovely yeah pinning still passed and I can see from the from the history that there was only it wasn't modified since it was had it, and it was added right before.

A

All of this started, so that's great, so the the pin verifies that it's not that the Alpert hasn't changed change compared to fine creepy changes, real drop downs, Co computer sex version, even sort of prop right. Okay, that'd be interesting to see part of that process.

A

I'll, so, okay, so he just now computer I.

A

Think that got moved actually into the was that still a proper can't remember.

C

Drop-Down delay out.

A

C

A

Version name so that actually stuck around for a bunch of tests, refactoring pinning test added, no.

D

A

Why is that listed twice? That's the same comment as that one, but it's not is it because it's actually.

A

D

Been a strange.

D

A

So I think there were two concurrent branches mm-hmm.

A

Maybe this is a bad rebase and yeah cuz. These are mostly exactly the same. Do you know what I can I can teach you? A really cool thing get ranged if origin/master and then I can do the first head of this and then the second head of this I know tell me the differences between them and you can see that they're all exactly the same. Okay, so why? Why is that I? Don't know what happened there?

A

I don't know interesting name, but what's okay, so that nineteen commits because there are nine in each branch and then there's the merge, commit yeah, merge branch.

A

Okay, so there's actually only nine commits really interesting.

A

Fine, okay I at this point would go through the comments that I've left- maybe reword, some of them, especially in this case, because I've been a bit terse but other than that I would be ready to hand this back to the author and say ask for their feedback. I. Don't think I'll do that now, because it's just gonna be boring to watch so I think we've done great any any thoughts or questions.

A

No thoughts, no questions green well,.

E

Good and pretty pretty long, it.

D

E

Long a night even I feel like I, do not understand the full functionality of them are so it's very hard like III go through the mr again and and they individual comments so that I could understand like what party was aiming for like the feature rise and yeah. It's really a big and for them I like.

D

Actually, thinking.

E

That respects the pinning part which I've never.

A

After this cool after I turn off soon, I will probably run this locally. Just so I can see the UI play with UI because I you know it gives it a lot more understanding, I think, especially when you don't know. What's going on.

A

And I will put links to these. This, mr, and also the mr, as talking about which describes pinning tests and attempts to document that.

A

Cool well thanks for joining, and both of you probably need to get to bed I. Imagine but I will stop recording. Thank you. Thank.

D

E