🚀 Space Reviewers 👾 Episode 5 - PR review deep dive Ticket 36352

I'm going to review a Django PR. It is for ticket number 36352. I have my notes in a repository. So you can go to github. com slash ontoi slash Django dash tickets. In that repository, I have in the README two links to the Django contributor documentation The first link is for a review checklist. This is really helpful. It walks you through all the different aspects that you should be looking out for when you're doing a review So definitely check that one out. And then there's another link to the git alias. So this is really helpful. You put this alias into your.

git config file and it allows you to do this command. Get PR and then the PR number and that allows you to easily pull down the PR that you're reviewing and switch to that branch. Okay, so my notes are here. So ticket number 36352. And as we go through the review, I'll be touching on the points that are outlined in these notes. We're going to start the review and what we're going to do is we're going to read the description and then we're going to go through the discussions. And we're trying to get an understanding of what this problem is, and then we also want to get a sense of direction of where

the solution is headed. Alright, so the description says was upgrading from 4. 2 to 5. 1 when tests were failing with a certain query in our app. And then they provided some models and they provided their queries. This throws an error in 5. 1 and the error says field error cannot select the foo underscore ID alias Use annotate to promote it. The current workaround is to include annotated field in values like so. So they had to uh update their query to include foo underscore id in values before they could use it in the subquery.

Okay, so for the disc the discussions, the triager Sarah is trying to validate that this is a legitimate bug. And she's trying to get more information from the bug filer. And eventually the bug filer provides more concrete models and Sarah is able to bisect the problem to this commit right here. And she also provides a more simplified reproduction code. with this query right here. And so this query says mapping dot objects dot values id. annotate foo And then F expression foo underscore underscore name. And then foo underscore ID equals to f

expression foo underscore ID. Okay, so here this is the part where we will be focusing on. So the problem when we look at the title. It says values raises a field error when multiple values of annotated values are chained. And in this reproduction example, um that's what the the the title is trying to describe this. So we have annotate and then we have multiple chains of values. So the first one is saying values and foo referencing the foo annotation that was provided here and then the second values is referencing the foo underscore id

which was provided in the annotate here Alright. And then another triager, Simon, is weighing in on this and says, it looks like we could error out with a more appropriate message. But we've made some significant changes since 4. 2 to prevent obscuring existing field references with annotations. So there's two parts to this message. The first part sounds like it's giving a sense of the direction for the solution, providing a more appropriate message. And then the second part is saying that the changes since 4. 2 was trying to prevent users from

I guess using f annotation names that might be confused with existing fields. And then down here, this is trying to expand on that and trying to explain why it could be confusing. For example, In other words, I would expect cause of the form mapping. objects. annotate foo equals to f expression foo underscore underscore name and mapping. objects. annotate foo underscore id equals to f of expression foo underscore id to always be problematic to some extent even when values is used as they turn follow-up references of to such annotations ambiguous.

It sounds like this is saying whenever you're using the name of a field that is a foreign key Or maybe it doesn't even have to be a foreign key, maybe it's just a name of an existing field, and you're using that as the name of an annotation, then things get confusing. So that makes sense. For example, what should annotate foo equals foo underscore underscore names dot filter foo underscore underscore name equals bar resolve to or What error should be surfaced now that foo is no longer a foreign key in the context of the query? Alright.

So this comment seems to be concerned about having foreign, um not not just foreign keys, again, so just having your annotation names be confused with existing field names. Alright, and then And then Sarah says, good point. Happy to have a more helpful error message. So now one question that comes to mind is. What would that error message be? I'm just going to put that down here. What would be a more helpful error message?

I don't know. what that might be um yet but let's dive into code let's take a look at this commit here and see why the changes were introduced and what it's doing So I have that opened up here. And then the ticket for that is number 34437. And we try to understand why this um why this change was introduced. So this ticket says when doing book. objects dot annotate annotation equals value one dot values annotation underscore type The resulting error field doesn't mention annotation as a valid choice.

And so this tickets it sounds like Before it was not including the name of the annotation in the list of choices. And the fix to that was to make sure that the list, um, to make sure that the annotations are included in the choices. Okay, so now we have we have an understanding of why this commit was um what this commit is trying to achieve

And if we look down here at the test, we'll get that sense of having that. um that annotation name in there. So it shows that it's expecting a message, cannot resolve keyword annotation typo into field choices R And then the choices are created here and it includes this annotation name. I don't like the way it was called here, but this This annotation, that's the name of the annotation. And now we're expecting it to be included in this list of choices.

that are presented in this error message. And we can look into the code to see how that change happened. I'm going to go ahead and check this one out. So I'm on the mean branch right now. I'm just going to check out this commit Alright, so we're on this commit now and I want to run this test and see what's going on. Okay, so I was on all right, let's see. So here we have our test

And I want to run let's see, I want to run this Pass if I run it now I know it will pass because it's been merged in and it should pass and So this is my command. I'm doing Docker Compose run and then I'm doing the Postgres container and then this is my test, the path to this test values wrong annotation If I run it now, it's going to pass. Okay, it passed. What I want to do is I want to go one commit

before this. And then I want to keep this test in place. So I'm going to copy this here and just drop it down here. And I'm going to do git log. I'm going to go one commit before this one. checkout all right where one commit before this one that introduced the bug we're trying to understand what the change was and how it works. Now I'm going to check, I'm going to add this test back in.

Okay, so I think it needs this, it needs to import this. Um import copy I'm just gonna drop it inside this test function here oops I didn't grab that Alright. Alright, so I have the test here and it should fail because we're one commit before. I'm going to run this test again Okay, it failed. And the

The message that we see is cannot resolve keyword annotation typo into field choices are annotation Oh no no oh sorry okay so let me do it this way let me remove this assertion thing let me just put that query there and then we'll just see that query so we're not doing any comparison to assert anything. I just wanted to raise that error. Okay, so this is easier to look at. We just have that error, it's raised, cannot resolve keyword annotation typo into field And then we look at the choices. It starts with authors, contact, contact ID, ID, ISBN, name pages, and we could see how it does not include annotation.

And so the commit that we were looking at, it's trying to make sure that this annotation is included in the error message. Now I want to I want to turn on the debugger and see how this is working. Um and one thing I could do is take a look at the code that was changed here. So if we expand this up. Okay, I'm trying to find I think it was add add values.

Trying to find all right this part so this is ad fields okay it was called ad fields This field error used to be raised inside of add fields, but it was now it has now been deleted. These lines of code have now been deleted And then if we find where ad fields were called, add fields. This is where in set values. So if we looked at the description of this commit it says

while the add fields call from set values does trigger validation which is what we saw here add fields it triggers this I guess the validation refers to this raising of this field error Wow, it does trigger validation. It does so after annotations are masked. So let's take a look at where add fields are called in set values. So we're in set values right now and add fields happen here um let me do highlight

all and here and that happens I'm guessing um this section here is where the annotations are being masked. It does so after the annotations are masked, resulting in them being excluded from the choices of valid options surfaced through a field error. So this is saying how these annotation names were masked. I don't fully grasp what that means. From what I know, there are masked and unmasked annotations, I think.

The Unmasked ones are the ones that are included in this select statement. I'm not entirely sure. It could either be that or um Let me see. I'm going to jot down a question. What does mass annotations mean? And then my guess, my guess number one is It means the annotation is not included in the select statement in the select list. Or it means the annotation

is being referenced by its name and not the expanded expression. I I'm actually I don't really know what it means. And I could try to find I could try to do a search to find out. So if I try to do a search on mask and try to get some context on it Um I don't think I get too much from it. I see like in this one It's mentioned in it shows up in a traceback, but that's about

it. And let's see what is going on here. Mask. So this says something about Nylevely clear the annotation mask as added to resolve this and this um this line So it's clearing the annotation mask. I still don't really know what the annotation mask is. And I could also do, let's see, I can do a search for Annotation mask. Masking

mask existing annotations that are not referenced by aggregates. Return the dictionary of aggregate columns that are not masked. And should be used in the select clause. I'm not too sure. But these are my guesses. Okay, let's go on.

So we were looking at we Mask nope okay nothing well abandon this search for mask and All right, this is Okay, this is not relevant or I don't know how to make use of it. Okay, so if we Go to here and I wanted to show how we can

start to understand what this change in this commit was So we have um set set values, set fields Add fields. Okay. Alright, so I am on the previous commit. I'm gonna go back. to the one with this change that introduced this bug. Oh okay.

Check out tests annotation test okay Check out all right and we know from looking here that Um this field error was removed from the add What was it add fields or yeah it was removed from the add fields method and And then these lines were added to the set values method. And I know that the error that we see

from this test is being raised in this line right here. And I'll show you. So we can go into here And we can see that the error is right here. Cannot resolve keyword into field. Choices are And available is the list of choices and it includes annotation select

So if I remove this, it would not raise the error I don't think it actually tests that line of code, but oh well. Um Let's take a look at this again. Set annotation mask. Set the mask of annotations That will be returned by the select.

Okay, um I think we can maybe see what happens if we let this fall through. I'll put a breakpoint right there And I'm going to run this code. I'm going to run it with this command where it exposes the debug port. And it drops into the bash shell And then I'm going to run the test using debug py So I have

my test here command and I'm going to use the test we want is Test values wrong annotation. And now I'm going to run the debugger. Alright, I'm removing that Okay, so we are now at annotation mask and I want to step into it and see what it is doing. So names is none. There's no names. I guess it's just not going to set anything.

Let's look at our cost deck. Where are we? Where is the test? Oh I'm a bit confused. I don't know why it doesn't show it.

I wonder if it hasn't gotten to the test yet. I'm going to put a I have a breakpoint there. I'm gonna just remove this right now and just run it. Okay, now I'm going back and putting that breakpoint back. And then we're gonna step into it or uh run it and let it hit. So now we're in that test, it's on values list And we are on set values and annotation names is empty So like

we could step into it, but it's just going to be empty. Um it wasn't masking anything, but I don't understand Field names include annotation typo. So that's field names are what is put into the values or the values list method Okay, so it's it should be raising the error in

add fields. Although I did not put the error back in Names does not include Uh okay, it does not include the annotation.

If we step back a bit Annotation names. Why does it not include annotations? And why is it that when we add this code back in What is it that allows it to include the annotations in there? Names to path. Alright, I'm going to finish running the code and then I'll rerun it with the With the code restored.

Okay. So it finished and then I'm going to restore this Alright now I want to be able to come into here and see what these values are. So I'm going to run this again Fail to save this. Uh let me

let me close this. Don't save and then I'll reopen it Um there we go. And then I just Um I take out this one and then I'll go back here and I'll add the the breakpoint back in set values. Where am I? I'm in the wrong one. set values I want to go into here okay

so lookup separator is underscore underscore F annotation typo Soft models dot meter And I want to step into this one. I want to get down to here. Okay. Annotation select. Um does it have a value? Self dot annotation select.

It does. Annotation. So that's how it was able to include it.

I don't understand this, but all I know is that annotation select is what is allowing this error to Include the annotation name in the error in the choices. Filtered relations All right. So that's I'm gonna put down a note. Names to path um from commit whatever this commit is

Copy paste paste. Name stup is where the Error is being raised that includes the annotation before the annotation is passed Okay, I don't fully understand it, but let's move on. So I'll finish running this code and then We were just looking at the bisected commit and now let's look at the PR that was opened. So here's the PR and there's no comments on it yet, and that was one of the reasons I chose

this ticket to begin with. Let's look at what has changed. Again, not too many lines of code has changed. Here's the test. We can see the first set of tests is very similar to the example provided in the description. I won't focus on that. I'm going to focus on the second test because it's the simplified version that was provided by the triager. And it's easier to work with this simple simplified test. Um Okay, and then we look at the code that they changed.

Remember, we kind of got the sense that the solution that is um that they want from this ticket is to have a more helpful message. When we look at the test, however, it's not actually checking for a more helpful message. And when we look at the implementation, it's not adding a more helpful message So what it's doing is it's putting a guard on this part of the code that raises the error And okay, I should probably

step take a step back. So let's check out this PR. Git PR Okay, and then I'm going to take one step backwards and go to the previous um to the previous commit similar to what I did earlier and I also want to preserve this test so let me preserve that test first Test and a tape. Um not this one a lot

So I'm going to copy and paste this copy Paste and then I'm going to go one commit back copy git checkout paste And now I'm going to put that test back in Copy paste. Okay, and then I'm also I'm going to ignore just complicated test. I just want to focus on the simple one.

So we can now run this and We would expect it to fail because the changes have not been put in place yet. So if I run this test I'm going to run it let me exit this container and then I'm just going to run it without the debugger And copy paste Okay, so it gave an error.

It says cannot select the author ID alias. Use annotate to promote it. So this is similar to What the bug filer reported the god cannot select this foo id alias use annotate to report it to promote it sorry And now if we put the test if we put the code back in, if I check out the commit Um check up PR and then run the test it should pass

But that's not any guarantee that anything was implemented correctly. Okay, so We know the test pass, but now let's try to dig in and understand what changes were made and if they're even the right changes. So let's look over here What changes were made? We remember the solution that we kind of this direction for the solution that we got from the discussions is that There could be a better error message. What we see in the changes in the PR, however, is not a better error message. It's trying to make it possible to reference

that annotation, which I think I I think that might not be the right solution. Um so I'm going to add a note. I actually already have it in my notes here. Review is the desired solution for the ticket to have a better error message. Or is it to allow the annotation to be referenced without raising the error? So I I have that question and Yeah, that's part of my review questions. What they've done here is they've

added a guard. To check if F is not in annotation select then They will raise the error and say we cannot select this annotation alias, use annotate to promote it. And if it is in annotation select Then they add it to the annotation names.

And then they also add it to the selected. So selected is a list, it's a dictionary of fields. And then there's also this these two lines of code where they are checking if there are any masked annotations. Then include it in the annotation names. I don't fully understand if this is a good if this is the solution for this ticket or not but we can run and see what values we get as we run through the test

So let's try that. Um I'm going to run it with the debugger this time and then we can put breakpoints and inspect the variables Okay, so let me grab my command to run the test. And then in my code, I'm going to I actually let me let me stop this. I don't think it ran the test yet, but I I want to remove this more complicated test and now I want to run it. And then for

we can start the debugger. I didn't put a break point. Oops. Let me go back. Um I can start that and then I go to the test. I'm going to add a breakpoint here I'm going to add a I don't want that. I want this dev set values I want to add a breakpoint here, although we'll see that it actually doesn't come into here. And that's also another point that I want to mention in my review. And I want to I want to know what's going on with this

annotation select mass. I still don't understand what that is Um okay, just those two points And I'll start the debugger. Alright, so it comes into here. Let's see the callback I think I did it a little bit too early, so let me put let me make sure there's a breakpoint here and then I'll just remove The breakpoint from

where is it? The queries, where did that go? Uh trying to find the queries. Okay, so this one okay and then I'll run it let it get to that point in the test and then now I'll add it back in Right here. And then we can run okay. So now we're in the test. We look at our cost

stack We 're in this test right here. It's on this line the first values chain and it goes into there and it gets to set value So we're right where the author added some new code. Let's take a look at this value. So this select mass includes both author name and author ID And so it will end up getting added to annotation names. I still don't understand the logic behind this. I don't understand it. Let's dive into here. So names has author name and author ID.

One thing I do want to point out about this test is they used um So authors is an M to M field. It's a many to many field. I don't think it matters in this case, but it's something I wanted to point out. And then they also their annotation is not Using a name that already exists So I think what I'm trying to point out is that the conversations here about how you're trying to prevent obscuring existing field references with annotations, I don't think that actually applies to this error message.

I think this error is being raised when you have the chaining not because of obscuring the names but because it's losing that reference So I think if I were to name this something else Let's say um my author name and then I'm going to name this my author ID I think we still see this problem. So I'm going to change this here. Change this here And and by this problem

I mean that the error can be okay. I'm kind of long-winded but let me restore the code again. Let me just finish running this test And then I'm going to go one, commit backwards. And then Did it do it? No, it upboarded. So I want to remove make sure I have a copy of the test. Okay

Oops. What did I do there? Get checkout I don't want the changes. Get checkout. Then I'm going to do one commit ahead one commit before this one. And now I want to add the test back in And I'll just work with the simple test And I'm going to change this value

my author and my author, my author And I'm going to run the test and it should fail. I'm just going to run it outside. Outside of the bash shell So we could see that it actually gives the error. It doesn't really matter if it's um if it's conflicting with the name or not that's not relevant here I don't think so that's one thing I want to note

Um de up scaring of annotation names with existing field names does not seem to be a factor in this ticket. Alright. That's just something to notice. And I can switch this around too and you'll see it will always error on the second one.

So it's saying my author name alias use annotate to promote it. So last time it was my author ID and we switched it around And that happens because when we look into the code here It is losing that reference to the annotation. I don't think that's an obscuring thing though. Because the name is not obscured when I've changed it It's just losing that reference.

And then the changes that the author of the PR made Is it valid? I am not sure. PR one nine four seven eight. Oh, uh check out P R one nine four seven eight Is it valid for them to just add it back to the annotation names list? And then to set the annotation mask to that list.

I really don't know. I'm going to um wrap up. So let's just take a look at my notes and see If there's any points that I need to cover. So we went through the PR changes. We looked at the We looked at this line. So it's adding a guard here. And then in the next few lines it's adding the field to the annotations annotation names list and then also into the selected Dictionary and so that's down here and then

A little bit further down we see it's adding the annotations to the annotation select mask. So that's down here So it's kind of trying to make it possible to reference these annotations by adding them back into these lists And then the test it adds in it adds in the reproduction example from the description which was the complicated one and then it also adds in the simplified test And they both pass, but then at the same time

we're not sure if the code implementation is actually the right solution. And then here I just had some questions trying to clarify and hopefully Um hopefully we were able to clarify so F in this code when we're looking up here and it's looping through the fields. So F is a field It's when you say like values and then so when you say values and author name, so that's what F is. F is referring to these What is selected?

I think these are fields that are going to be added to the selected list. And then what is annotation selected? I think these are the annotations that are going to be included in the selected list. So that's as opposed to the annotation select mask. Um and I'm I'm still not sure what this means. I'm not sure if this is right. I I don't think I understand this. Um What is annotation names? These are just the string field names and it's trying to keep track of

the list of annotation names I believe and then here's just a little bit of reading that I was doing on the documentation. Um so that was when you go to the documentation it explains about annotation. I was trying to understand Like what is annotation and then it also explains that an alias is similar to an annotation, but you would want to use it when you don't need to include the results of that annotation in your in the return values of your

select statement. Um so I just added that note in there and alright so I'm going to get to my review. Um, there's still a lot I don't know, so I'm still nervous about doing this review, but I think I have enough to put some comments. The main question is this one is the desired solution to have a better error message, which I don't think the PR implemented a better error message Or is it to allow the annotation to be referenced without raising the field error, which I think that's what the PR does. It's allowing the annotation to be referenced And it's um

avoiding that raising the field error. And then I just had a question like for These lines down here maybe my my question is not really valid anymore. I'm not entirely sure. I was saying why um Would there be any case where F would there be a case where F is not an annotation yet this logic is adding it to annotation names?

I'm not entirely sure. I don't think that is a valid question. I'm going to remove this. Okay. And then while the test passed changes in lines are not being covered by the test. Does this need another test case? Okay, yeah, so I don't think the tests are actually testing this section of the code because it doesn't actually enter into here. Um So that's something I want to mention. And then on tests. py line. This is just a nitpick could it use QS.

count instead of length. So in the test right here it uses the length I I just suggested that it could use qs. count instead of length. Um Okay. I'm nervous because I I still don't understand I understand the problem and it's not exactly what I don't think it's what this discussion is about about obfus um obscuring annotations with existing field references. I unless I don't understand. Maybe I don't understand. I don't know. But I'm going to go ahead and leave a comment.

Let's go through the checklist and see if there's anything else we need to cover So here's the checklist and we can go to each section documentation. I don't think this bug needs any documentation because it's not changing any behavior of annotation or or the values methods so I don't think we need to worry about that Bugs, is there a proper regression test? The test should fail before the fix is applied. Yes, we did see this. It does have the proper test and this was taken from the description and also

the simplified reproduction code that was provided by the triager If it's a bug that qualifies for a backport to the stable version of Django, is there a release note? Bug fixes that will be applied only to the main branch don't need a release note. I believe this qualifies for a backport. I'm not entirely sure. But what we can do is we can go and search for the term upgrading to see if there were any other tickets that have these similar upgrade problems where bugs started to appear. So let's look at the first result and we see They were upgrading to Django 5. 2 and they encountered an error

and this was for version 5. 2. So let's look at the PR to see if they added a release note. Oh look, they added a release note for this bug fix. Alright, let's take a look at a couple of others to see if they have added that as well Um this is close bug invalid. So this was not a bug fix, it was invalid. Um let's take okay so that's won't fix, won't fix, close invalids, close fix. Okay, so this one is a closed fix And if we take a look here, the version was 5. 1. It was happening when someone tried to upgrade to 5. 1 and they encountered this bug.

So let's look at the PR for this. And see if they have a release note. So they do have a release note for this bug. I'm getting a sense that this ticket will require a release note. Let me just put that down. Does this require a release note for the bug bug fix? Alright. And then new features. It's It's not a new feature, so we don't need to worry about that. Deprecating a feature. It's not deprecating a feature. We don't need to worry about that. Does the code styling conform to our guidelines? Black, black and dogs, plague eight, I

sort. If we look at the CI, we can see that the tests for these linting are run. So there's black 8, i sort, and then I believe this Um maybe that's blackened docs. I'm not entirely sure, but it does have something to do with documentation. I'm not too worried about it because the tests pass on CI. If the change is backwards incompatible in any way, is there a note in the release notes? I don't think this is backwards incompatible. It's just a bug fix. Is Django's test suite passing? Yes, it's all passing. Is the pull request a single squash commit with a message that follows our commit message format? So this is an example of the commit message format

You have the title and then you have the description. And this has the title but no description. Um I I think that's okay. So it looks like it follows the format Are you the patch author and a new contributor? I don't think this is a new contributor. They've contributed several PRs before, so I'm not too worried about this part. Does this have an accepted ticket on track? It does, yes. Alright, so we've gone through this checklist and the one thing that stood out was this having a release note for the bug. So I've added that to my little release, my review section. Alright, so now

let's add this other one. This is the main question that I have that I want to ask in the review Is the desired solution for the ticket to have a better error message or is it to allow the annotation to be referenced without raising the field error? And then this question, I don't think it's a question. I'm not entirely sure anymore Um oh I see I see I see what this is asking and this is kind of related to the um this is saying does this logic actually ever get Does it ever actually get to this logic? Because it says if F is not in self.

annotate select, then raise this error. But would it actually ever get there if it's already checking um Wouldn't it uh always get into there? So if it doesn't pass this one Why do we need to check it again? Why do we need to check if it's not in here? I don't think we need this check. So that's one thing I want to do. I want to add um for this line here does it need

To have the guard. So that's my question. Does it need to have the guard And one way to test that out is I'm going to remove all the changes in this code. So I'm going to remove this guard. I'm going to remove these here. I'm going to restore the original code. So those were the lines that were added. I'm going to run the test and it should fail. And then I'm going to add this guard back in. And I'm guessing it will actually fail again. So I'm running the test.

Let me see. Okay, I'm just focused on this simple one right now All right, it failed. Okay, so now we can put the card back in and see that it's still going through this path of code Alright, so it's coming through this path of code again. line 2523 and it's failing again. So let's go ahead and restore this and see if uh let's see what happens when we do that

Um it should pass Okay, so it passed. I don't think it's coming through this line of this block of code here. If I don't put that in, it should still pass. It's just not coming through this block block of code because now I think um let's see where is it going I think it's coming to here if it's an annotate select So I'm going to put um I'm going to I'm going to put a print statement here. Um and let's see if it comes through there

Yeah, so it's coming through this one because I don't exactly know where Annotate Select is being set. But it's being set somewhere and it's now that this these two lines of code are added in the the code is coming through here But ultimately what I think I am oh mmm it's it's going through there because I think is it because the annotation select mask is not none anymore and it's returning some values? Okay. So that might be it. And then let's go back to the code. Um

I don't actually know so I don't know why this I don't know if the guard is necessary. Um Especially with this logic, this line up here, because if it's if it comes into this block of code, then it doesn't really reach down here, but if it doesn't Come into this block of code that means that F is not in annotate select which means that this would always be true. So I don't think it's necessary Um, so that's going to be, yeah, that's a note that I have, it's not necessary. And now What do we want to do?

Um I think I'm I think I'm ready to write the review. So if it gets into the block of code In lines two five two one With the guard, always evaluate to true Because the condition on line 2519

has already evaluated to false Alright, I think I have that comment down and then does this require a release note and then all right I'm just gonna put in a little introduction. Hello I reviewed this PR and have some questions and comments. Is the desired Or is it too allowed to build? Um, do I need to expand on this one? Okay, I want to add in a link to the comments

and that will just give it some legitimate see so I'm going to go to here and copy this comment and I'll just link that here to have a better error message to have a I'll go ahead and use the wording in this comment so a more helpful To have a more helpful error message or is it to allow the annotation to be referenced without raising the error I think that's all I'm going to say. And I think I'm ready to write my comment. Okay, so I'm going to select this

block of code here. Does it need to have a guard? If it gets into the block of code in lines one. to one two five two one to one two five two six with the guard always evaluate to true Does it need to have the guard online um two five two two? If it gets into the block of code in lines 2521 to 2526, would the guard always evaluate to true because the condition on line 2518 has already evaluated to false. Alright, I'm going to start a comment

and then I think I should be able to finish the review and I'm going to copy and paste. So I'm saying hello, I reviewed this PR and I have some questions and comments. Is the desired solution for the ticket to have a more helpful message? Okay, or is it to allow the annotation to be referenced without raising the field error? Does this require a release note for the bug fix? Um, is there anything else I should say? Okay, I'll I'll add in um thanks for the patch.

I um should I tag this person? Okay. I did an initial review of this PR and have some questions and comments. Alright, I think I'm ready to hit submit now. I'm I feel like there's still a lot that I don't know and Hopefully this is just the start of the conversation. I'm going to hit submit.

Okay, um, I did my PR review I'm going to update my I think I'm going to update my notes and we'll see how this goes. Right, well I can't wait to see w what feedback I get back from the PR author and also from the any other conversations with the um on the tickets here too. Hurt data