🚀 Space Reviewers 👾 Episode 3 - Django - Backward operation for RemoveField enhancements

And

Speaker 1: and uh thank you for joining us. Uh today we have uh a new member of the queue. We are now a fellow Sagent team and today we are going to make a code review about another ticket that we have truths and our uh mainly goal is to uh is to help people to um to make um a better code review and and also show our process and how uh we make uh some de ch some decision and uh and how it it's um and and how um we hope to help you as well

Speaker 2: Yes. Yeah. Um, hi, I'm Tim. I'm one of the other space reviewers. And Sage, would you like to introduce yourself?

Speaker 3: Hello, yeah, I'm Sage. I'm a member of the Django community, I suppose. And yeah, today I just this is my first Django Night Space review reviewers session. So Let's see how it goes.

Speaker 2: Woohoo. Yes. Yeah, so this review session is typically we take a PR that is in some various state of review. Uh today we are looking at a PR that is handling a migration operation called remove field and we're it's supposed to add a default to it. It'll be a little bit different than what we've done in the past. in that the code and documentation for this look pretty good, but I think we will have to go through some more of the um evaluation of the code, which we we've tried in the past, haven't made it really far, so I think this will be a nice mix-up for us.

Speaker 2: And Raphael , she already explained, we do this to help expose the process, try to make it a little bit more accessible. Yeah, hopefully this helps people understand how some other people might approach code reviews. Um and so you can use that in your own professional world or review some Django PRs too. There go ahead, Rafaela.

Speaker 1: Oh can I? We also have would like to um to make a call to help us to uh uh choose uh a next uh PR that we could be uh use in our next online review and you can you can find out this uh this form inside our repository or um inside the maybe inside of description of the YouTube channel

Speaker 2: That is a good note. I don't know if we let me Yeah. If we don't have that, we'll have it next time. Um yeah, and the last thing is please be respectful. We're operating under the Jenga Space Code of Conduct. The link is also on um a repository if you have not if you're not familiar with it. Alright. So with that We have our little show notes for everybody to look at. It identifies the ticket, the PR, and then our summary of the issue. And so the goal here is to kind of

Speaker 2: Pull out the relevant bits that we need to know or that you need to understand.

Speaker 3: I think the zoom controls is um obstructing the view a bit.

Speaker 2: Oh. Oh okay, thank you.

Speaker 3: Cheers.

Speaker 2: Um yeah, so we have the Episode notes here and we try to identify the main pieces of information that you're going to need to understand to do a reasonable code review. Um so we sometimes some tickets It can have a very long um set of comments. This one is not one of those. We probably didn't need to summarize all the discussion, but we did. Sage, would you mind introducing the ticket as you understand it?

Speaker 3: Oh. Okay, so I think this is about the migrations feature in Django. And when you do migrations, uh a migration in Django consists of different operations that we um that we add to the operations list inside the migrations class. And uh so Django by default has built in um Operations such as add field, remove field, and for example also like create model. And these are the operations that will be performed on the database.

Speaker 3: That's what will be uh that's how the changes will be made to your database and migrations can be uh executed forwards and also backwards. So for example, if you add a field to your model and you run the mic uh sorry, if you just added the field to your model in the code and then you run make migrations that's going to generate this um migration file uh which has this migration class and also a list of operations and this is only a defin the definition of what will um what

Speaker 3: will be changed in in your database, but um it will not actually run until you um execute the migrate command. And when you might when you run python manage. by migrate, that's going to execute uh the operations in order. And So um the operation has um one second. Um So yeah, that's okay, nice. Yes, so in here you see the SQL Statements that will be executed when you run the migrations.

Speaker 3: And you can do that by using the SQL migrate command. And this is the forward operation, which uh yeah is is the changes uh that that will be made to the database. But an operation can also have the Backwards um operation.

Speaker 2: Sorry, I don't I never remember it's backwards or reverse.

Speaker 3: Right, yeah. So this is the reverse or like the backwards operation of that same migration. So uh for a create model operation, which translates to creating a table on the database. The reverse operation will be sorry, the backwards operation will be dropping the table. And yeah, if you change the field name of a model , that means the reverse will be changing the field name back to the original name before the change. And the ticket is about

Speaker 2: yes. Hold that back up.

Speaker 3: Right. So The ticket is about the backward operation for a remove field, which removes a field from your bottle, and it doesn't allow uh default value if the field is uh not null. So it it also has the um null equals false in the definition So yeah, uh as you can see in the description, the backward operation will break if the database is populated because If you refers if if if you refer a remove field operation, that's going to add the field back. But currently before uh with this

Speaker 3: tickets still open. The issue is that the field will be added, but it will not have any values even if you have a wait one second. It is it um when it has a default, like is it ignored?

Speaker 2: Um so I I think the default in the past was used. I think this is about catching the cases where there isn't a default and yeah, trying to alert the user like, hey, your backwards migration won't work.

Speaker 3: Right. Yeah, so currently Django will just proceed to um create a migration and if you try to run it, um Django will not have any handling and it's just going to break, I suppose. if you try to run the referrus operation because yeah it adds the um column but um it cannot populate the uh the column. So yeah, it's going to um trigger the not null constraint on the column.

Speaker 2: Yeah, yeah. Databases are tricky like that where they expect if you say it's not null, you can't create a column with all nulls. Um Okay. Thank you for that overview. Um let me get rid of that. Um yeah, I don't know So uh the discussion here I thought was pretty interesting. I'd if Florian and Simon had a discussion, I think this was years ago. But basically they wanted to do a very similar thing as like with AddField, where if you try to add a new field on a table that also has roll rows and you're adding a field that is not null

Speaker 2: that it tries to it pauses and says, hey This is going to fail when you run the migration if you don't specify default or set a value for all the rows as it's this column is being added, or you have to make this a nullable column. And then I think there's also the option to just say. Yeah, I know what I'm doing. Ignore this Django. Um and so Florian pointed out that we should be able to do this in the reverse direction too, of having somebody be aware of this when you're creating the remove field operation. That we could slow the person down and say, like, is this really what you want to do? Um That that that really stuck out to me. Um But yes. So We have the PR

Speaker 2: now All right. Yeah. So this one is a pre um I think Devil's Autumn is also a Django Not space person. I believe they re-based it on or based this on this other PR from Marcus.

Speaker 3: From 2016.

Speaker 2: Yeah. Um

Speaker 3: and the ticket is even older.

Speaker 1: Nine years.

Speaker 2: Yeah. Um all right. Do either of you have a recommendation on how we want to proceed with this? So it looks like there's been a fair amount of review on this already. I haven't looked at it that thoroughly, but also I trust these five people quite a bit. Um Did did either of you look through the code ahead of time?

Speaker 3: Only uh like I I I only skimmed it for a bit. Um yeah, I think there are different ways. You can approach a PR, you can just straight into uh jump straight into the code or um like um either look at the implementation or sometimes it's also useful to look at the test first to because sometimes the the test describes what is expected, right? And it also demonstrates the issue which ideally the test should fail before the fix is implemented. So that means we can prove that there's this issue that you can see by running the test

Speaker 3: and after the patch is added. the test will pass. So yeah. You can either look at the implementation or in some cases it's useful to look at the tests so that um you know where to start.

Speaker 2: That's a good idea. I like that. Rafaela, any any things that you wanna check out, review, make sure we touch on?

Speaker 1: Uh I think I I I noticed even because I also um saw the previous PR that it was uh open but uh but close at a certain point and I I I think I noticed that uh my sequel uh is populated with the and not raise uh an uh an error that uh it you probably um will will expect uh in this kind of uh operation I think.

Speaker 2: Okay.

Speaker 1: Inside the test.

Speaker 2: Yeah. I am curious. I remember when I was looking through so check SQL. Is that roughly right? Check the SQL to operation.

Speaker 1: Test operation.

Speaker 2: The test. Okay.

Speaker 1: Test remove fields and unnullable

Speaker 3: Yeah, and and I think that issue was specific to MySQL. So yeah, you you might encounter cases where a different behavior is observed on certain database backends. And in this case on MySQL , according to the comments here, um MySQL populates not null columns with implicit defaults, like um I'm assuming, for example, if you have a text field or a chart character field um with without a default and you add a column with uh not null

Speaker 3: constraint instead of failing MySQL will just populate the column with like empty strings for example

Speaker 2: That's interesting.

Speaker 3: Yeah, so um it's also very useful to have the um I think it's Docker Django developed.

Speaker 2: Yeah, I don't trust Linux to run Docker and Zoom at the same time for me. Yeah. That's that's gonna crash immediately.

Speaker 3: Yeah. I'm not saying that you should run it right now, but Um yeah, if you if you're trying things out on your own, it's very useful to use that as the starting point. Um because It has all the databases set up for you so that you can run the tests with different database backends. So yeah, if you write a test um and then you run it with one database backend, sometimes it would just fail on some others. Although um yeah if if it's not a feature that's um like directly related to the databases, it's usually The same.

Speaker 3: So just running it on a single database. is fine and you can also just let the Django CI do the work. But obviously um in cases where you actually implement a feature that works differently on different databases, you would want to run the test locally as well.

Speaker 2: Yeah. So yeah, the UR, it's Django slash Django dash Docker dash box. Um we'll put it in the show notes. I'll add it to the episode notes as well. Yeah. Um, but I find I'm a little surprised that That there's a check for the vendor here? Um, because what you were explaining kind of sounded like this isn't going to err. So this wouldn't have It would never raise um the integrity error here. Oh, never mind. This is a test. I'm sorry.

Speaker 3: Yeah.

Speaker 2: Okay. Yeah. I thought this was in okay. Yep. My mistake. This makes a lot more sense.

Speaker 3: Yeah, but that that's a good point though, um, because sometimes you also don't want to specifically check for the vendor here because um Django also supports custom database backends, which are not in the Django package itself, and it can be implemented as a separate package. If you create such database backends, you also want to run the test, um, like the whole Django test suite with your database backend. And if we have like spatial uh special cases like this inside the test itself and

Speaker 3: If you have if if you're writing a database backend that also has the same behavior as MySQL here. Uh since we hard code the vendor here, um it's not going to pass like the the test will fail right here because um Yeah, uh it's it's hard coded in the in the test. And in some cases, Django avoids this by having what's called the database features uh flag.

Speaker 2: So

Speaker 3: um inside your database backend code uh we have a list not a list well um We have a bunch of attributes on the uh database features class. Um I think it's in features. py Yeah, that one. Yeah, so these are like the default flags for all database backends. And if you build your own database backends, you can subclass this database features and set the flags appropriately. And then you can use this flag to check in the test instead instead of checking specifically for the vendors.

Speaker 3: But what gets a flag is uh not ever not everything gets a flag here because there's a lot of different um behaviors and just quirks, I guess, if if each database back and that it it's not worth it if you have all of them as flags in here. So in some cases, um there's also I think if you scroll down a bit more uh oh uh if you scroll up just a bit yeah there's um Django test expected failures and also Django test skips that uh you can use to point to the specific tests that you would want to uh skip or expect it to fail.

Speaker 3: for your database backend so that you don't have to um batch Django itself to to skip the tests

Speaker 2: okay wow I had no idea that existed um Yeah. Okay. I was gonna say, is there a reason like should it using like a a decorator on this of

Speaker 3: Yeah, normally you would check like connection dot features dot um the the f feature flag name.

Speaker 2: Oh I meant more I've seen it in the past where there's a decorator here saying skip if.

Speaker 3: Right. Oh yeah. So Django also has um skip unless DB feature. And also skip if db feature, which I think is just a wrapper around the built-in unit test, skip if, and skip unless.

Speaker 2: That's the test for it. Okay.

Speaker 3: Yeah. Uh you might need to I I think if you look for it using camel case instead of um snake case.

Speaker 2: Ah.

Speaker 3: Yeah.

Speaker 2: Thank you.

Speaker 3: Yeah.

Speaker 2: Okay.

Speaker 3: So yeah, it's it's a handy wrapper around the built-in skip if and skip unless. that um automatically uses like uh the database features. And so you you just need to pass the string of the feature flag to the decorator.

Speaker 2: So there's nothing Okay. Okay. For some reason I was thinking like it would be possible if we're gonna do a check on the vendor. Um like this, there would be a decorator that you know annotates the test method uh and would say like valid vendors

Speaker 3: Um I I don't think Django has a specific decorator for that, but I think it should be um Fairly simple to just use the built-in unit test skip if. So for example, you would do at skip if and then you put the condition in there, which is like connection. fender Yeah. Equals MySQL, for example.

Speaker 2: Okay. All right. Well, we've talked about that quite a bit here, which is a very small part of the PR.

Speaker 3: Exactly.

Speaker 2: Cool. Alright. Uh do we wanna take some time and like five, ten minutes and read through the the code? Do we wanna dive into like running the tests? What are both of you thinking?

Speaker 3: Um I think I saw you have a local like small project to demonstrate the issue. Maybe it's easier to do that. And Um yeah, so we want to make sure that the issue is still there and not like um because in some cases the issue might be fixed somewhere uh in uh some other VR and it's just the ticket hasn't been updated. Okay.

Speaker 2: Anything we should do before that, Rafaela?

Speaker 1: Before uh before starting the um the the the project the the example that you have in uh in mind?

Speaker 2: Uh to show I I think we're Sage just suggesting is we go through the problem case and kind of show what the problem is in Django that this ticket was created for and then We can from there just, you know, pull the latest from the the PR and confirm like, all right, did this work in the most simple of cases?

Speaker 1: Yeah, I think also the inside the PR just made some some example of but I think it's more uh Um it's more useful if we can see the difference between

Speaker 2: Sounds good. All right. Um Okay, so I have this little example app. I use it for a bunch of other things. It's actually a little bit based on Django models. I don't know if I even have any data. Um it should be all caught up. So I want to add Okay, so we uh I'm gonna add this index field to question.

Speaker 2: I think I just yeah. So this is on the ad side, which is fine. Let's go ahead and ten. And so yeah, as a foreshadowing, this is what the one of those comments was from I think forget who. They were talking about this preserve default. So we've added that. Um I don't know if I have shell plus installed. I do, which is from Jang Jang Django dash extensions, and then I have

Speaker 2: iPython, which makes things a lot easier. It automatically imports stuff. Um and so I did it on question. I've already forgotten what fields are on this. Text update index Okay. So now we should have objects.

Speaker 2: Of course. Cool. We have an object, we have a row there. So that should be enough. And we said we were gonna remove the field. So we have now removed field And now the case that they were talking about is if we try to go back to zero zero four. It should break. Yes, there we go. And so I'm running SQLite with this for if anyone was wondering. I um But yeah, so it failed here on not null constraint

Speaker 2: failed because we have that row that I just that I originally created up here. That's actually I'm curious what that sequel is. Yeah. I always forget SQL has to drop the whole table and like recreate. Yeah. So it's recreating this index. is not null and then when it inserts yeah it's literally trying to insert null into it which would explain why it fails

Speaker 2: Um right, so what is the best way to go backwards from this? Suppose maybe we can do is delete the database.

Speaker 3: That works.

Speaker 2: Right. So uh now we want to use uh okay. I'm I'm just skipping a few steps. So now we want to use the code from the PR. in my local project to run through that same use case and see what is different. And so to do that Oh, it's up right there. I just clicked on something. So GitHub provides this nice little GitHub CLI. And there's other ways you can do it with Git. We're not going to go through that, but if you're curious that that you can do some research on Git. But for now, we'll run GitHub PR checkout. And so it switched us to this branch. It should be from Devil's Autumn.

Speaker 2: So we're on the right thing. So now this Django folder for me has the latest code and we can install That package with pip um with the dash e. I don't know what dash e actually does. I believe it means editable.

Speaker 3: It does, yeah. Okay. So if you don't use Dash E, if you make further changes to the local cop copy of Django, like for example, if you switch between branches, you'll have to reinstall it. That if you have dash e it will automatically be updated.

Speaker 2: Okay. See, I've never even tried installing a local instance of a package without dash e like this. So I I've never Ran into that, I guess. Um but okay, so now we've installed the dev version. Um We're gonna have to undo some of these. Or no, I can migrate to four.

Speaker 2: There's all the other Django we'll see if this breaks. I think it might. Oh, nope. Okay. It's complaining because I removed the field.

Speaker 3: Oh yeah.

Speaker 2: Okay. So now should say we have it in our database. So now we're back to where we were before. Now if we remove the fields, we're gonna use the same migration. Oh no, that's what we need to test. It's making that migration. There's our nice little message. Anyone have a suggestion on which one we use? Let's go with one.

Speaker 2: Micrate. Okay. So it's still creating that column is not null, but this time instead of null, it's going to insert 15, which should do everything, right? Oh, yeah, all those other migrates were from Django, but the one we care about is right here. And so now if we migrate backwards again. Um so now we'll have the field. Sorry it got off re-add the field to the model.

Speaker 2: So we've migrated backwards to where the question now has index back on it. And so we should see 15 as the value for index. And I believe

Speaker 3: What was the original value before we dropped

Speaker 2: Five. Yeah.

Speaker 3: Five, right.

Speaker 2: So yeah. So I shell plus and i i python, like it provides like the history. And so yeah, that's I forgot to. That's why I was immediately looking that up. Cool. So that piece looks like it works, which is really cool. I wonder I want to look at that message again. Um So now I'm I'm trying to put myself in the shoes of a new developer. Like say you're just using Django for the first time. You don't really understand the relationship between models, migrations, and your database, like how they're all three moving parts. Is this going to be confusing? Because like it's not talking about your forward migration.

Speaker 2: It's talking about the backwards migration. Um not recommended to remove it. Not in the I'm creating backwards. I don't know.

Speaker 3: This is very trivial, but I notice a typo there. After the dot after the first sentence, there's no space.

Speaker 2: Good catch. Right. Notes. We can show the the GitHub suggestion feature.

Speaker 1: I think I default. Uh

Speaker 2: Rafael?

Speaker 1: I I I I I saw inside the questioner that it's not um I I think it should work uh I don't know why In the because I saw the mm The ask uh not null removal that uh the this uh message come from and there's a um There's a pond dot and then there's uh uh okay, okay, okay, okay. I noticed that it's not uh

Speaker 1: slash uh N for um for going in another for like um for making the space

Speaker 2: Um I'm not sure I'm file. Should I be looking at the code base?

Speaker 3: Yeah, I think Yeah, I think we can look at the typo in the code later after um but yeah at least we've we've made a note for that so yeah So I guess right now we are trying to assess the clarity of the message for Newcomers, especially?

Speaker 2: Yeah. Yeah, that's that that was something that crossed my mind of that might be a little confusing. Um I don't know if you can make this that much better. Remove a not nullable non-nullable field. I mean arguably this should be the to remove the non-nullable field. Unless this has a

Speaker 3: It might be based on the message for the other one. I forgot what that one was.

Speaker 2: Yeah, it's a minor thing. Um because something needs to populate the existing roles when migrating backwards. Yeah, I wonder if like it's worth trying to s like it's alluding to the fact that When you remigrate backwards, you would be adding the field back. And that field needs to have some sort of. . say I wonder if it's better for us to call that out explicitly of saying like rather than implying like hey the remove field when you migrate it backwards is gonna add a field

Speaker 3: Yeah, because I think especially for newcomers, um, they might not know that you can even run migrations backwards. Like yeah, so what does migrating backwards mean? It may not be clear. So yeah, I I get what you mean.

Speaker 2: I don't know if it's worth making the change or like suggesting it. Like I I don't know how verbose we should be here. Um Because I think like once you see this once and you understand it, like you're only I only ever look at like what what these messages are. I don't read this anymore. Um Yeah. So I'm gonna put a note. We can come back to that.

Speaker 3: Maybe it can be as simple as adding like the phrase. So for example, this is just an example. This is because the database needs something to populate existing rows when migrating backwards, which will add the field back. 2D model or something like that.

Speaker 2: Yeah, I think we would probably want to have I I agree that is probably as simple as it could be or as it needs to be, but I would probably put as a separate sentence. of like when migrating backwards, period. When you migrate backwards, it's going to re add the field. Hey, Shafia's here. Yeah, I think that would probably be uh what did you say? So I agree.

Speaker 3: I think we're talking in terms of models and fields, so maybe we add it to the model.

Speaker 2: Is that but then yeah we the if you

Speaker 3: I'm not sure. Yeah, but yeah, it's it's just nitpicking. Um

Speaker 2: I'd actually feel kind of strongly about not using the word model there. Um but uh yeah.

Speaker 3: Yeah.

Speaker 2: I don't know. We'll we'll see what happens when we get closer to it later. Um Okay. That that satisfied my need to be worried about that aspect. Um okay. So now we know we've confirmed that this works here. I think the other note we had was to force the test to fail to make sure the test is actually doing something. And then checking the SQL on the test. So should we move on to those? Is there anything else we want to test in the application, the CLI?

Speaker 3: Well, I mean you can also test each of those uh options like uh what happens if you choose number two? But

Speaker 2: that is true. I'm hoping that the tests will have like the unit test would have that.

Speaker 3: Oh yeah. That's that's also a good point. Yeah.

Speaker 2: I suppose we can review that for completeness. Um we said This is gonna be difficult. I think I'm gonna try going full screen. There we go. We're not gonna have the notes for now So there are hm okay. So test commands. I just want to see is it worth switching out of this view?

Speaker 3: I think it's the cogwheel icon at the top in the middle. Um in the in the sticky header, yeah.

Speaker 2: Thank you. Yeah, okay. This might be a little bit easier. Okay, so test migrations, not null field

Speaker 3: Yeah, you can see there's no space after the period, and this is uh using the I'm not sure what the name is, but I tend to call it implicit string concatenation, where if you have a um parenthesis and then you have a bunch of strings in there without having a comma in between the strings it will be automatically uh concatenated but yeah in here you don't have the space after the first sentence so that's why um we have the typo

Speaker 2: So the thing I wanted to show where is it?

Speaker 3: It's the first button, I think. Yep.

Speaker 2: Add a s no, this is the one I want. Uh so add a suggestion here. I I like I like this in GitHub because you hit this, it brings in the line, and then if we add the space, it does the diff for you. And then the the person um Say I don't well I'm not gonna have the ability to merge this, but like what it would look like then um the author side is you'd have the ability to like batch all the suggestions up and commit them in one go, which is I I think is really helpful on code bases where like there are going to be some nitpicky comments. Like this one doesn't re deserve anything besides like the subject. I don't think we need to comment anything besides You know, showing the suggestion of

Speaker 2: a missing space. Okay. I'm just gonna add that. Um okay, so I think is this test checking the messaging?

Speaker 3: Yeah, it captures the standard output.

Speaker 2: Okay. So I don't know if this would do arc which is good. Like this t tests the CLI. I don't think this tests the Um actual functionality, like what what you were saying earlier, Sage, of what happens when you type in one, two, or three and making sure like the full integration.

Speaker 3: I think that will be in test operations. Okay.

Speaker 2: Okay. That's not removed.

Speaker 3: Yeah, I think it's also interesting to see um because now you can also infer uh the structure of the Django code base, which means that the CLI part is uh located in in a separate part of the code base and it's tested in different ways. So um for the for testing the CLI that's the only thing you do you just test the what it prints and what happens if you input um one of the options and for the actual operations you test it um you have a separate unit test that doesn't involve the a C L I at all.

Speaker 2: I think that makes sense. Right? Rafael, any uh uh

Speaker 1: I would like to ask you uh Sage for So basically we have two type of texts, one with uh uh that involve uh CR the CI and one who don't. Am I correct? Correct.

Speaker 3: Um so one tests the um CLI interface uh that uh pops up when you try to run make migrations where it asks you the question. And it doesn't actually check, like for example, um the SQL that's generated and also whether it will uh raise uh an integrity error, for example. That part is not tested in the test commands that we just saw. And that um yeah, we have a separate test for the migration operation itself. If that makes sense.

Speaker 3: I guess it's um it will be clearer once we start um reading the test for the operations.

Speaker 2: Yeah. So I I was waiting to see if there was a a follow-up question to that. Okay. So excellent. There is only one method here. I just wanted to see if it was It's not exactly easier to read there. Cool. Okay, so this is the test operation. Okay. Project state. I'm gonna ignore that for now. Create an operation model. Must create the data. So like this is when we did our question with index equals five.

Speaker 2: And which field were removing? Wait, okay. That's funny.

Speaker 3: Nice one.

Speaker 2: Yep. Uh all right, so migrate it forwards, which would remove the weight. And then this is what you were talking about, what we talked about earlier. For MySQL, it's gonna ignore Trying to confirm that it's gonna air. So this is so is this effectively testing that databases do error?

Speaker 3: Yes, um on databases other than MySQL.

Speaker 2: Okay. That's interesting. It's like it it we are effectively if one of those databases were to adopt MySQL 's approach of having an implicit default instead of raising an error, we would then know about that in our main Django would know about that because main would start breaking.

Speaker 3: Yep.

Speaker 2: Okay. Interesting.

Speaker 1: So Sage uh probably in this case uh uh it It's uh we we are we maybe could use the decorator that we talked about uh or um instead of uh checking for vendor.

Speaker 3: I think if we want to use the decorator, we'll need to have a database feature flag on the database feature class that we discussed earlier. Yeah, I'm not sure like the decision on what exactly gets added, like whether this justifies a feature flag is something that's um Usually I I'm I I might be wrong here, but I think it's made on a case by case basis. And Yeah, if if somebody has a particular concern, they would usually raise that during review, like whether this justifies a database feature flag or not.

Speaker 3: and the other reviewers will chime in and um add their opinions and if everybody well if it's agreed that it should have a database feature flag, then it will be added. Otherwise, you might see this specific vendor checks in the tests.

Speaker 2: So is there a reason not for us not to adopt something like this where we're doing the check for Psycho PG three versus two?

Speaker 3: Right. The only reason might be might just be that you'll have to write a separate test method for that. So you might have to duplicate the the entire test and then remove the assert races.

Speaker 2: Well, okay, so hold on, let me skip if

Speaker 3: connection dot vendor.

Speaker 2: Right. Well, so I mean if we had a property here of uh So that would Oh, I guess we don't want to skip the entire test. You're right.

Speaker 3: Yeah, so

Speaker 2: it's just that one part.

Speaker 3: Yeah. So you'll have to duplicate it or like structure it in a way that it's tested separately.

Speaker 2: Which I I don't know if that's really

Speaker 3: It's it's not a problem. Like uh it's it's perfectly okay to to duplicate it, I think. Um

Speaker 2: well yeah, especially because like this this part is doing This is like a canary for how do databases work. This isn't evalu I I don't think this is evaluating anything with this change. It's just to see how do databases work in the future and do we need to make a change. So like should that be a separate test method? And I I don't know enough about Django to know

Speaker 3: Right, so I think one way to see it is this way. Let's let's see uh if if we imagine removing the if uh check so that we only have the assert races. Yeah, so this is the behavior that we ideally want across all databases. So if you try to run it backwards, it's going to erase an integrity error But yes, so like I in an ideal ideal world, this is just how the test is.

Speaker 2: Right, but but um I think my my arg or my my my question is like this is Do tests are tests supposed to be written where they serve one purpose? Because this is testing two things. One, it's testing the the operation itself. Oh, I guess not. Okay, never mind. I'm getting confused here. I've

Speaker 3: So without the default, it will raise an error. And then a after you add the default, it doesn't. But yeah, since on MySQL, even if you don't specify the default, it doesn't raise an error. That's why we had the check. But um I think One potential issue with this is that if MySQL in the future erases an error, Django will not know about it. Which is not necessarily a problem. It's just that um we have this check that um

Speaker 3: end up not being used and actually is harmful if you now expect it to erase an error because yeah Django just ignores MySQL entirely when checking the integrity error.

Speaker 2: Sorry, there was somebody joining.

Speaker 3: I think um I'm not sure if it's possible, but Um a possibly better way to do this is uh to check that MySQL actually uses a um uh an implicit default and maybe have that as a feature flag like um for example uses implicit default for refers or some sort of flag right and then in the test you actually do check that the um inserted

Speaker 3: uh column like uh includes that implicit default rather than just skipping this um case specifically

Speaker 2: Yeah. third party packages with database backend, implementing a database backend, which is the one that I can think of as Adam Johnson 's MySQL one. Or is that MySQL?

Speaker 3: Um because MySQL is built in and the Django MySQL uh package, it doesn't add the database backend, it just adds like um f features that are specific to MySQL, sort of like Django Contrip, Postgres. Was

Speaker 2: Ryan. I was thinking of MySQL Jenkins. But yeah, I also said Adam Johnson and I've written yeah. Anyway, yeah. I thank you for the uh the clarification on um uh Adam Johnson's package. Um so I if if we're trying to make that that experience better, if you the I'm trying to st think of the case where If someone else has a if they're implementing this for a vendor where this this would run because they don't have MySQL 's connection. vendor, this rate does not raise the exception. Is it possible to catch that and ignore it or like how would what is that

Speaker 3: Yeah, that's that's a good point. So what happens is that the test will fail because it doesn't raise this the exception and we are asserting that it actually erases. So it's unfortunate But uh yeah, so they will have to explicitly skip this test or mark it as expected failure, which is not ideal. So um Yeah, either we break this down into separate cases. Um for example with the default and without the default and then so that at least you can break it down into two separate methods. But Yeah, I think ideally

Speaker 3: you would have a feature flag here. But usually um if it's like a very specific quirk We don't add it until someone makes a case for it. Like they submit a PR. Hey, can you make this into a feature flag, please?

Speaker 2: Okay. Yeah, it doesn't seem like it's used a lot, but it is used. So I I think Yeah. Okay. Anyway, uh so I'm I'm okay with not mentioning this. I think I would really like to have this in main and rather than getting it bogged down on this particular topic.

Speaker 3: I think well I I don't know if there has already been conversations on GitHub about it.

Speaker 2: No, it doesn't I think we missed part of that code earlier. Um I suppose we could check the other PR see if Did this handle

Speaker 1: I think uh they yeah, they um use the same approach

Speaker 2: Yeah. Yeah, I I think I'm leaning with um wait for somebody to implement it. They can ask a question. It doesn't seem like there's a lot of cases like I don't think this is something Django needs to start fixing or like start addressing. Um Oh. Oh right, that's yeah. Okay.

Speaker 3: Yeah, it is uh quite uh we have quite a few places where we do that check, so it's not uncommon.

Speaker 2: Okay. Good old Oracle.

Speaker 3: Yeah, if you if you count how many vendor-specific checks, Oracle might be the winner.

Speaker 2: Fun, yes. Um okay, so we have operation default and then it confirms this is what we did earlier of confirming that it was 15 coming back, which is good. So if all those tests actually pass, that's a win. Um Yeah. Does this Where are we? You said not operations. Oh, this is the wrong PR. Getting lost. Is there a what is test loader? No, I don't think there actually is a test for so Sage

Speaker 2: earlier you you pointed out um Do we need to test something for each one of these cases? Um that it translates into the right thing. But I wonder if that's part of the questioner Ask not null. Yeah, where's the test for ask not null removal? Is this I might be missing something.

Speaker 3: So this can we have a look at task commands again? So that's so yeah, it tests that uh with option three, it raises system exit, which means it quits. And with option one, you provide the default and Yeah, it doesn't test the second one, I think. Oh wait. Um is it w what about yeah, uh what never mind. Yeah, I don't think it tests the second one

Speaker 3: You would need uh another mock dot patch for built-in's input with return value of two.

Speaker 2: I wonder if there's Is there another test that does this? That does something similar. Interactive not null addition, not null removable. No, I don't think there is. Yeah.

Speaker 3: I mean, I guess we can see how it t how the test for the um adding a new field uh with without Sorry, adding a new non-nullable field is which should be a few lines above this. I think it's

Speaker 2: this little

Speaker 3: bit.

Speaker 2: So two quits, one provide defaults.

Speaker 3: And those are the only options. So yeah, ideally we should also have uh Assert uh some assertions for when you um choose the second option for for this one. Yeah.

Speaker 2: So no, there is another one that so this is the not null alteration. Three one. Okay, so this is what it's mocked at or modeled after.

Speaker 3: Yeah. I'm not sure if it's possible to test. Um I think output don't get value. Hmm.

Speaker 2: Because then It should be a successful exit from the command.

Speaker 3: Yeah And I'm not sure if there's a way to capture the so if you look at the questioner. py file Oh I forget the yeah I think it's yeah. So I think um for the second option yeah it just returns the

Speaker 2: Not provided.

Speaker 3: Not provided. I'm not sure if it's possible to check that value is. I I'm not sure if you have access to that value from the test.

Speaker 2: It does seem like we need to have a test here though in questioner tests. So like we have one test asked, not null, not provided. So I think that's what we're missing is this test. Um

Speaker 3: yeah, that's exactly it. Yeah.

Speaker 2: Okay. So let's add that to the notes.

Speaker 1: Can I ask you something?

Speaker 3: Yep.

Speaker 1: Inside the test command, there's a comment that says no message appears if try run

Speaker 3: Right. Yeah, so um if you run When you run the make migrations command, you can have um there's the dry run option and I think it doesn't have any output if if you use it. Like it it I mean it's not going to ask you what to do, I think So yeah, we can try that maybe.

Speaker 2: I wanted to show what the description was first, which uh just show what migrations would be made, don't actually write them. So Wait. Oh. Interesting. Okay.

Speaker 3: Yeah, so that's going to be the same behavior as the previous one where So it's like the option two where it would just proceed with um making the migration, but in this case the migration file will not actually be created.

Speaker 2: Are either of you familiar with use cases where dry run is common, like commonly used? Like I

Speaker 3: I think a common use case is to run it on CI to make sure that any model changes already have their corresponding migrations created.

Speaker 2: Minecraft.

Speaker 3: So yeah, if you if you change the model without creating the migrations. Um That's bad because um yeah your migrations do not reflect the final state of your model and you would want to detect that on your CI so you can run the dry run migrations on CI.

Speaker 2: Okay. That makes sense. Anything else down that avenue, Rafaela? Uh

Speaker 1: sorry?

Speaker 2: Uh anything else on that topic that you wanted to ask or discuss?

Speaker 1: No, I was wondering because the the I think This is the test about the questionnaire. And then we just see that the the option three is uh is tested, option one is tested, but then I notice this type uh So I I was curious about this.

Speaker 2: Gotcha. Uh yeah, so I because we just found like, hey, this method isn't tested, um I'm now curious, like what are the other changes doing? Um So this deconstruct method on so this is the remove field. Not provided. Okay. So I'm going to imagine this is very similar to oh this is the field operation. Very similar to the add field operation of default not provided.

Speaker 2: I do remember seeing this in test operations at the very end. It was confirming the deconstruct. Yeah, it was testing if the deconstruct works. I find it interesting sometimes of how the I never know how to write my tests of should I have a test for each single evaluation or should I combine the tests? Like so like in this case It's testing that the database like it actually the database actually macro uh migrates backwards and like is there an error or not? We test um whether the default works and then it's also testing deconstruct and like this deconstruct seems a little out of place. Um, but also there's probably

Speaker 2: tens of thousands of tests in Django. Like, do we want to add another one? I

Speaker 3: Yeah, I think one of the other considerations is that to check the deconstruct, you will need to do the same setup like um

Speaker 2: oh

Speaker 3: and it's yeah it's just not worth it sometimes

Speaker 2: that makes sense okay that answers my question then of yeah Yeah, no there's no read to no reason to run the same test setup, but

Speaker 3: okay.

Speaker 2: Okay. This is backwards. So this is what we were testing earlier. Um I remember seeing a question from Sarah Boyce on how does database default work with this? And I don't Where is that? Oh, maybe it was oh apparently it was Lily. Um

Speaker 3: I think there's another thread somewhere I forgot. Um

Speaker 2: see if we can find

Speaker 3: further down below, I think. Uh uh did you know that if you press Alt click on the show resolved, um Yeah, if you press Yeah, it's going to open every single thread.

Speaker 2: I did not know that.

Speaker 3: Yeah, so it's possible to control F from that point.

Speaker 2: So this was brought up Is this a marked I don't think this one's marked as resolved. So I think they uh Okay. I'm just trying to figure out like now with expanding all of them, which are the relevant conversations, which ones aren't No, so that was marked resolved. So there should be a test.

Speaker 3: I think there's another one. Um there's another thread down below. Um I'm not sure why it's not I can see it on my I think it's the first review from Lily.

Speaker 2: This one?

Speaker 3: Yeah. Um yeah, exactly. Yeah, yeah. That

Speaker 2: re-adds database defaults. Can we? Um I think we might want to go check to see how adding a database default works. Like how do we test that? Like do we actually inspect I mean

Speaker 3: I think

Speaker 2: I guess it would be part of the remove field, wouldn't it? That wouldn't be part of this.

Speaker 3: Yeah. Um Yeah, it it well I think so the way you would add a db default is at the point of the add field operation right and then when you do a remove field it should look at the dbdefault before the removal happens and make sure that it's actually used And I think it might already be handled um because hm Well, it's just going to reuse the previous definition.

Speaker 2: Yeah. Right. Yeah, this has that that shouldn't be related that shouldn't be related to this PR. I think when they added support for db dot default, that's when you would need to cover remote field. I wonder if you hmm Would you ever have a default and a different DB default? I mean not logically you shouldn't, but Do we break on that?

Speaker 3: Hmm, that's a good question. So

Speaker 2: now I've

Speaker 3: I'm not sure if Django allows you to do that.

Speaker 2: Yeah. SQL Lite support Db default

Speaker 3: Oh, so I think I'm currently evading the documentation. Um So you can have both db default and field default. And the plain default will take precedence when you create instances in Python code, but db default will still be set at the database level or when you add for for things like when you add a new field in a migration.

Speaker 2: Oh shoot, I did this backwards. Please no one take any notes on how to manage your database through this example. Not what you're supposed to do. So now we've added Okay, it didn't give us the warning because we have the default set, which is cool. Um that's something we did not test. Adding okay. That all makes sense.

Speaker 2: Default ten at all. Okay That is kind of interesting. It is all right. So it's using 10. What did we So default was zero, db default is 10. So it's going to I don't know if that really matters to us, but it looks like it's going to be using the database defaults for the migration.

Speaker 3: Yeah, because the default um the old default, not the DB one, it's never reflected in the SQL statement as the default clause It previously was just uh computed in the application level, like on the Django level, and by the time it gets to the SQL query, it's just the value, for example, zero or ten. Yeah

Speaker 2: I suppose if you wanted to over I'm just thinking like eventually someone's gonna be like, no, I don't want the db default, I want the default Can you just set it that way? No. I feel like you should. Right? Maybe it's because it's falsely. No that's interesting.

Speaker 3: I think it's it's consistent with the um documentation that says the dp default will still be set of and it will be used for inserting rows or like when adding a new field in a migration because technically when you reverse a remove field that's going to be an add field. So yeah, uh the plain default is only um It only takes precedence when you create instances in the Python code, but when it comes to migrations or if you somehow insert

Speaker 3: VRO outside of the ORM. It's still going to be used. Yeah, I'm I'm not entirely familiar with the DB defaults. So I might be wrong.

Speaker 2: We have this wrong. It's it Okay. Alright. Alright. I I s I think I'm getting a better understanding of it. I think in the case of someone did want to override this, it's probably gonna be Oof, yeah, that would be weird. Like maybe a separate database and like the separate model state and database. Yeah, that would not be fun. Um okay. Anyway, that's a very

Speaker 3: What they could do is update the previous def uh migrations where the DV default is Add it change the value from there, I suppose. Or or yeah, use the uh separate database and state maybe. I don't know.

Speaker 2: Okay. Anyway. Um Okay, so we checked that auto detector. Do we need an explicit test for this? Do we have one? I don't think I don't think we have a test explicitly for this, but I don't know if we need one.

Speaker 3: Um can we have a look at how the test operation is set up again? Okay.

Speaker 2: So it's going through the happy flow.

Speaker 3: Yeah.

Speaker 2: Not we're not test test uh excuse me. Evaluating all the cases of this if statement. I was wondering if maybe somebody else on this call or on the video in the chat wants to comment in, but we'll see.

Speaker 3: So there is a test auto-detector file, but I don't see it in the PR, so I'm not sure if There should be a test there. This is a very long file, I think.

Speaker 2: Yeah, probably not worth going. Um so I think The other way to go about is could we find a similar evaluation or similar code and see if it's tested extremely thoroughly or not? Maybe this one instead? I don't know. I'm just guessing at Yeah, that's what we just added. Well, that's the only one that does that.

Speaker 2: Okay, so there's some logic here. Added field. I think we'll get another one. My brain's turning to mush.

Speaker 3: Yeah, I'm currently looking at the same code as well, trying to find whether um yeah that might be it. Yeah, so I found Uh one second. Test add not null field with

Speaker 3: yes, this one.

Speaker 2: Yeah, this looks doesn't look like it's covering every single possibility. So I I think we're good. I think the PR is fine, in my opinion. I don't what do you each of you think?

Speaker 3: Um I would probably have a look at um the previews. um like how we initially added for example like um adding a field without the default with a null equals false and like whether that has a test in the autodetector. Because yeah, even though we we can test it manually, but it would be nice to actually have the test in the auto detector because that's That's a crucial part. That's what links the

Speaker 3: test commands, the bit that tested the command line and the bit that tested the operation. because both can still pass if the auto detector didn't work, I think.

Speaker 2: This might be a rough Code change to look through part. So you don't so that's pretty clear. And so this is the PR where database defaults was added. And yeah, it it was one test.

Speaker 2: I'm not sure if that answers your question, Sage. These two tests from when the

Speaker 3: So I guess what we um you you can look for, for example, in the test auto-detector, you can look for mentions of the questioner. Yeah, so that would be testing the cases where it would normally um very uh like run the questioner and yeah uh I I think it would take some time but I would go through all these existing tests and then think how essential it is like to to have a similar test for

Speaker 3: this PR.

Speaker 2: All right. Um I think we're probably I don't know it how much longer both of you want to go looking at this. Um

Speaker 3: Um I do need to take a five minute break in sometime in the next 30 minutes if possible.

Speaker 2: I was actually suggesting maybe we wrap things up now. That

Speaker 3: that works for me.

Speaker 2: Rafael, any any uh last minute questions?

Speaker 1: I think we just cover a lot of uh with this PR.

Speaker 2: Yeah. Um okay. I agree. That that was a really deep dive into the code. I don't think we've gone Looked at the code that much in the past. Or definitely I've never ran this that many things in the terminal for one of these before. Um Okay, so the notes that we have So we added that one, so that's done. Not deleted. Um Right. The note about migrating backwards, if we want to try to be a little bit more clear, because that is a potentially advanced topic or like it's not a clear topic at least um for beginners

Speaker 2: um adding a test for That's similar to ask not null alteration not provided. I think that's probably a requirement. And then we have some extra homework to do Um yeah. Anything else that we missed?

Speaker 3: I don't think so.

Speaker 1: Maybe just uh with the dogs, uh Yeah. But

Speaker 3: oh yeah, that's a good point. Yeah, maybe we can quickly look at the docs preview maybe.

Speaker 2: Okay. Um do scroll up Um

Speaker 3: I think if you control F or read the docs um without spaces I had it open. Um one second, let me

Speaker 2: Yes, please help.

Speaker 3: Oh, where is it?

Speaker 2: I do think the docs change are fairly minimal. Yeah.

Speaker 3: Okay, so this is the build. Um but I haven't got the exact page up. Should be in reference

Speaker 2: Yeah, it's missing this is this is the old version. Um Yeah, why didn't this hmm

Speaker 3: hmm?

Speaker 2: Oh, let's try that. Change it to the right PR. There we go.

Speaker 3: Yeah, nice It looks different to the actual docs though, because um this is the one that's hosted on Read the Doc the Docs.

Speaker 2: Yeah.

Speaker 3: Makes sense to me.

Speaker 2: I think it makes it I think this might be two.

Speaker 3: Two, yeah. That's that's a good point, yeah. Well, um I guess it's because there's always already a two before that. So two populate and make.

Speaker 2: Yep, you're right. Raphael, any any thoughts on this?

Speaker 1: I try to read uh room once or twice, so I think it's probably clear. I I don't know if it's clear because I know what is going on or because it but i I think it's also easy to alter it.

Speaker 2: Okay.

Speaker 3: Yeah, I think the the first sentence saying bear in mind having that in the documentation I think justifies having more explanation in the message.

Speaker 2: Because

Speaker 3: it's something that's that may not be immediately clear.

Speaker 2: Yeah, I wonder if this actually like Rather than having the first part of the sentence be bear in mind that when, like should this be when reversing this mind when reversing a remove field operation, this is actually adding a field to the model where like it might be easier to pick out visually if you're skimming it

Speaker 1: Okay, we are also um um reviewed because I saw in the review just uh uh um last pre um three row that it was updated because uh the first uh the first sentence is it It was uh from it was just a bill uh already there.

Speaker 3: Oh, yeah.

Speaker 1: Hmm.

Speaker 2: Adding so Ryan says adding a field back to the model Yeah. This is also like weird. When reversed, it's not doing anything to the model, it's doing it to the table. And like even down here it says recreated column. So should this be actually This is actually uh adding a column back to the table.

Speaker 3: I think if we want to um evaluate the field versus column terminology I'll I would have to um read through the other operations docs to make sure it's consistent. Maybe there is no.

Speaker 2: Oh yeah, because we call it remove field, which makes sense. But at the same time, if we're saying what actually happens

Speaker 3: We're either saying about we're we're either either talking about what actually happens or what the reverse operation is in terms of operations and models, fields and so on. So yeah, it's documentation is not easy.

Speaker 2: No, no, it is not. Um I have another comment, another thought. It will ask for a default value to populate. Uh so it will It will ask to provide a default value, so instead it will ask for a default value to populate. Take both of these out.

Speaker 2: I do think the latter probably makes a little bit more sense.

Speaker 3: And more concise as well.

Speaker 2: Uh that is from our chat and I do not know how to credit you, but thank you. Oh okay. Sweet. Okay, so ignoring the field and model concept here, I I do like um Ryan 's opinion of saying adding a field back to the to the model. Um do we want to suggest changing this part? I don't I wasn't sure where you both of you landed on starting a sentence like that.

Speaker 1: Yeah, I think uh the starting of this sentence is strange, but uh I don't know if there's any Hmm. Any any other thoughts in mind when uh he was hard to that? Maybe we can just uh propose a a subtraction. I don't know if this maybe this could also be related with this PR.

Speaker 3: I don't mind changing the first sentence, but I also don't mind leaving it as is. So yeah. But I I do agree that um I think the fact that the existing documentation has that emphasis on what happens when you reverse the operation. I think that justifies adding more explanation in the common line

Speaker 2: In the the command output you mean?

Speaker 3: Yeah.

Speaker 2: Okay. Yeah. That's that's actually a really good argument that um yeah. Do you want to leave that comment, Sage?

Speaker 3: Sure, yeah.

Speaker 2: All right, cool. So I'm gonna put you down. Sage will do this. Rafael, do you want to add, since I already added a comment, do you want to add a comment about adding a test and test questionnaire similar to Uh test not ask. So uh sorry, I'm trying to pull this up.

Speaker 1: Test questioner.

Speaker 2: So the test questioner file has a test here under the questioner tests. And this is for not null alteration not provided, which is what sorry, I'm Where is that? There was no that I mean I'm all over the place right now. Okay, so in questioner there's this not provided choice, which we don't have a test covering that. And so

Speaker 2: ask not null removal is very similar to I'm in the wrong file. So like this one is very similar to our not null removal and In test questioner, there's a test for this not provided case that wasn't covered in test uh test command. Did I explain that well enough?

Speaker 1: Nope.

Speaker 2: I saw the smile.

Speaker 1: Okay.

Speaker 2: No, nope, I have not explained this well. Um Alright, so we have test commands here. Let me We have comments here of like which so it prompts the user for like which input they they should enter and like what happens. And so this test covers three and one, but there's nothing for two, which is ignore for now, make this potentially non-reversible, which is the not provided. Um, but that test should exist on test questioner of like this. Um like test not is it

Speaker 2: I think this is like what we need to add.

Speaker 1: But we have the this test inside the Um the the test command just is similar, no, or no?

Speaker 2: Uh no, it it well it's similar in that it's testing the other two returns. from like when the user gets prompted. It tests for one and three, but we don't have a test for two. And so we need to add this test explicitly to test questioner to cover that option. And it would match how we do things for when we alter a field that's not nullable, which is this other test method.

Speaker 1: Okay , can I can I read you what I wrote uh to to understand if it's understandable.

Speaker 2: Yes, one second, let me finish this Okay, yeah. If uh do you wanna put it in the chat?

Speaker 1: Oh yeah. Father. So

Speaker 2: Inside the test questioner. We should write a similar test. I would add inside the test questioner. py file. Um And we should write a subtest similar to yeah, that method name to cover the cover The

Speaker 1: the case uh the case from

Speaker 2: Yeah or the case, yeah. I um to cover not provided case.

Speaker 1: To cover the not private

Speaker 2: case. Yeah.

Speaker 1: Games where um

Speaker 2: That discussion we had earlier of adding a field to the model that that is not even in the changes here. So probably shouldn't be making that suggestion then. That is not yeah, not our place. All right. Check. Got that. All right. I think that's everything. Inside test question, we should write a test similar to

Speaker 2: test SNAT and I'll cover. Yeah, I think that's thorough enough that they will understand But that points them in the right direction, points out that hey, we're testing the not provided case. Um I think they will understand like, hey, these are very two similar paths.

Speaker 3: Yeah, I agree.

Speaker 2: Okay. So then when both of you are ready to submit those comments.

Speaker 3: Oh, we do eat now?

Speaker 2: Yeah, yes.

Speaker 3: Okay.

Speaker 2: So uh are we both ready? So there's a little thing, Sage, that I don't know if you've seen the end. uh of how these space reviewer episodes go, but it

Speaker 3: Oh yeah.

Speaker 2: Yeah. So there's when you push review or submit, you gotta scream. Sorry, everybody else.

Speaker 3: Explicit mention. Oh so that's in Okay. Um

Speaker 1: Sorry team, this time I just accidentally pushed the comment button before Everyone. I will join you anyway for the for the celebration.

Speaker 2: Yes, celebration. Not nervous at all.

Speaker 3: All right. I'm ready.

Speaker 2: All right. Uh I'm just gonna submit this without approved. All right. Rafael, you want to count us down?

Speaker 1: Three?

Speaker 2: So after after one it's a scream.

Speaker 1: Ah uh sorry I I was I won't understand if I have to come down.

Speaker 2: Yes please.

Speaker 1: Three, two, one

Speaker 2: Well, okay. I think that's everything. Um we have a couple extra notes that we'll add in. I I think Yeah, I don't know. I I'm not sure if we have any anything else to say.

Speaker 3: I just want to say I really enjoyed this. It's been great and you both did a great job. Like especially you, Tim, like trying out all these um on a on a actu on an actual project like like running things. Yeah it's it's really useful to see the issue and like replicate it and uh verify that the fix actually works. So yeah thank you for doing that.

Speaker 1: If I if I can I would like to uh also thank you Sage because uh you uh you really uh know where some where we can find something that it could be useful for for example the where to find the inside the the test or or for example the the the flag if it's needed. It it was very I really I really enjoyed it also because you you did a um Um a great uh uh a great suggestion for us.

Speaker 3: Thank you. Yeah

Speaker 2: Yeah, I don't know if we we can have the uh on the the slide of we're not all experts. I I think um yeah you were answering quite a few expert level questions there, Sage. I I think yeah

Speaker 3: , oh that's What spending too much time on the code base of TP? But yeah, uh we'll we'll get there eventually. So

Speaker 1: with your lead

Speaker 2: Yeah. All right. Well thank you everyone. Um if anyone's watching this in uh as a contributor to Django and you want to be a navigator for DjangoNot Space, please reach out to contact at DjangoNot. space. We are always looking for more navigators for Django or third-party packages within Django. But other than that, yeah. Take care. We'll see you next time.

Speaker 3: All right. Thank you, everyone. Bye-bye.