Itm 421 validator updates

[Anchita14]

I implemented a map to keep track of all scene characters and update/remove them according to the persist_characters flag and removed_characters. I created the validate_persist_characters and update_characters methods to manage character persistence accurately. Additionally, I enhanced the scene validation logic to ensure characters marked as removed are not present in both the scene's state and action mappings.

[nextcen-dgemoets]

Oh I forgot to mention-- I think these changes should have been in a new feature branch off of main, e.g., itm-466-persist-characters, not building off of the old (merged) branch, itm-421-validator-updates. I don't think we have to do anything about it (unless @kaitlyn-sharo thinks it should be fixed), but normally we create a new feature branch for each ticket.

[kaitlyn-sharo]

Yes, this should have addressed the approach you mentioned, Darren, and it should have been in a new branch

[nextcen-dgemoets]

Yes, this should have addressed the approach you mentioned, Darren, and it should have been in a new branch

Do you want this moved to a new branch (branched off of main), or just keep it like this and use a new branch next time? And do you want the character matching dependencies addressed here, or moved to another ticket?

[kaitlyn-sharo]

I think it should be addressed here. That was part of the ticket, right? If it was in the ticket, definitely address it here. I suppose this time it can stay in the branch, because it doesn't matter too much

[kaitlyn-sharo]

Updated character_matching function to handle persist_characters:

• if it is the first scene, or if the 'characters' properties exists in the scene, character mapping is handled the same as before. Look for all the locations where we might find a character id and make sure that id that's found matches a character id explicitly defined in that scene (or, in the case of the first scene, the scenario)
• if persist_characters is true, see if the character ids defined in the scene are defined anywhere in the yaml. If so, pass. Otherwise, error
• if a character has been removed at any point in time (and persist_characters is true), offer a warning to ensure that the character will be available in that scene

Logging has also been updated to define "errors" and "possible errors" instead of "warnings" and "info"

In the future, we will be investigating LOE for following scene branches to get a better idea of real errors here, instead of flagging every removed character and allowing every character that exists at any point in the scenario

[nextcen-dgemoets]

Some thoughts:

• So, does this mean we should remove validate_persist_characters and update_characters?
• "if it is the first scene, or if the 'characters' properties exists in the scene, character mapping is handled the same as before"
  • this only works if persist_characters is false/missing, right? The scene could refer to a persisted character.
• Warnings are in purple, and warnings count is purple, but errors are in yellow and error count is red; summary is red and green
  • We could put warnings in yellow (in both places) and errors in red (in both places); then put summary in green. Thoughts? I may be missing the initial intent of the color-coding.
• typo: locataion
• In character_matching, could you rename first_scene to first_scene_id? Seems a little clearer / more accurate.
• "Character ID appears in 'scenes[1].action_mapping[1].character_id', but is removed at some point during the scenario. Make sure that this character will not be removed before this scene"
  • Extremely minor, but could you change "will not be removed" to "is not removed"?
  • Should we use determine_first_scene in scenes_with_state?
  • It also looks like we could combine scenes_with_state and check_first_scene, but that's arguably beyond the scope of this ticket.

[kaitlyn-sharo]

Some thoughts:

• So, does this mean we should remove validate_persist_characters and update_characters?

I don't think so? Anchita wrote those functions, I was just doing the character_matching part. Do they overlap? I'll have to look...

• "if it is the first scene, or if the 'characters' properties exists in the scene, character mapping is handled the same as before"

  • this only works if persist_characters is false/missing, right? The scene could refer to a persisted character.

Can 'characters' and 'persist_characters' coexist? - update: thought about this a little more and it makes sense that they could. So what I've done is added a check so if persist_characters is false AND it's the first scene or 'characters' exists, we do it the old way. Otherwise, if persist_characters is true, we are still looking through all of the characters in the entire yaml so nothing changes. CHANGES COMPLETE

• Warnings are in purple, and warnings count is purple, but errors are in yellow and error count is red; summary is red and green

  • We could put warnings in yellow (in both places) and errors in red (in both places); then put summary in green. Thoughts? I may be missing the initial intent of the color-coding.

I really want to avoid errors in red because that is a LOT of red and I find it hard to read. Colors don't even come up the same on everyone's terminal. The intent of the color-coding is so that I can have a little fun :) I would be willing to put the end warnings as red or yellow if count > 0, but that's about it CHANGES DENIED

• typo: locataion

Okay, found it and fixed...it was only a comment CHANGES COMPLETE

• In character_matching, could you rename first_scene to first_scene_id? Seems a little clearer / more accurate.

Definitely. CHANGES COMPLETE

• "Character ID appears in 'scenes[1].action_mapping[1].character_id', but is removed at some point during the scenario. Make sure that this character will not be removed before this scene"

  • Extremely minor, but could you change "will not be removed" to "is not removed"?

My thought was that it is possible for it to be removed, so we want to make sure it cannot be removed, but I think either one gets the same point across, so I'll change it CHANGES COMPLETE

• Should we use determine_first_scene in scenes_with_state?

I had looked into this when I was working on my portion of the ticket. It's done a slightly different way so I decided to leave it, but I can change it too. Again, doesn't really matter. CHANGES COMPLETE

• It also looks like we could combine scenes_with_state and check_first_scene, but that's arguably beyond the scope of this ticket.

I'd like to keep these separate because the first scene tends to have a lot of separate rules from other scenes. Generally, I've been able to put these rules in the dependencies.json, but the point of check_first_scene is actually to make sure that the first scene follows all rules. It's just coincidence that right now the only rule is that it must not contain state. It used to have 3 rules in it, but I was able to separate out the others. I'd like to have it as a place to put additional rules in the future CHANGES DENIED

[nextcen-dgemoets]

Some thoughts:

• So, does this mean we should remove validate_persist_characters and update_characters? I don't think so? Anchita wrote those functions, I was just doing the character_matching part. Do they overlap? I'll have to look...

Well if nothing else, validate_persist_characters is no longer invoked. And I believe it is covered by the removed_characters character matching rule.

• "if it is the first scene, or if the 'characters' properties exists in the scene, character mapping is handled the same as before"

  • this only works if persist_characters is false/missing, right? The scene could refer to a persisted character.

Can 'characters' and 'persist_characters' coexist?

Yes, if persist_characters is true, then any characters you specify in persist_characters are added to the scene's characters (replacing any characters with the same id. I'm not sure that's well-documented (for users, let alone devs, so I'll look to address that).

• Warnings are in purple, and warnings count is purple, but errors are in yellow and error count is red; summary is red and green

  • We could put warnings in yellow (in both places) and errors in red (in both places); then put summary in green. Thoughts? I may be missing the initial intent of the color-coding.

I really want to avoid errors in red because that is a LOT of red and I find it hard to read. Colors don't even come up the same on everyone's terminal. The intent of the color-coding is so that I can have a little fun :) I would be willing to put the end warnings as red or yellow if count > 0, but that's about it

I agree that a lot of red is hard to read.