[REG-2230] Operate on BotSegmentLists so we can do validations there #374
Conversation
| // Also run the validations for the botsegmentList if they exist | ||
| foreach (var validation in _botSegmentListValidations) | ||
| { | ||
| validation.ProcessValidation(nextBotSegment.Replay_SegmentNumber); |
There was a problem hiding this comment.
So this is passing down the segment number, but the validation is not really part of that segment, it is part of that segment list as a whole... do we think this could confuse the user? This is why I used -1 for sequences, and then in the logs that gets converted to "SEQUENCE"
There was a problem hiding this comment.
i want to see the active segment in the logs as this runs so I can see what segment I was on as the validation was running
There was a problem hiding this comment.
they are absolutely being evaluated during that currently active segment, and should log as such
There was a problem hiding this comment.
if you want to also log that they are 'segmentlist' or 'sequence' validations as part of it, that would be fine, but I really want the active segment # on the front for all log messages
There was a problem hiding this comment.
Sounds good to me 👍 I can get behind that logic
|
@RG-nAmKcAz this LGTM, and works with my sequences and bot segments but I have not tried bot segment list yet... let me just try that real quick |
|
@vontell feel free to merge (will go into your branch) ... as soon as you're done testing / happy with it |
| @@ -192,7 +209,7 @@ private void ProcessBotSegments() | |||
| { | |||
| if (_nextBotSegments.Count < 2) | |||
| { | |||
| var next = _dataPlaybackContainer.DequeueBotSegment(); | |||
| var next = _dataPlaybackContainer.DequeueBotSegment(out _botSegmentListValidations); | |||
There was a problem hiding this comment.
@RG-nAmKcAz quick question on this actually regarding the lookahead logic. Does this mean that the currently running validation will be lost and replaced? I am not full understanding this code yet but I wanted to check if this means that validations that are currently running and not yet finished may then be running forever, since now we only operate on these new validations. Maybe we don't need to worry about that because we run StopValidations on the data container itself which does hold onto the reference...
There was a problem hiding this comment.
i'll huddle with you on this
Stop I think we're fine
but you correctly point out that we may not be considering all the proper validations... I want to discuss if we get rid of this look ahead logic all together or not
|
Closing this PR as it has now been merged into the other feature branch from #375 |
Find the pull request instructions here
Every reviewer and the owner of the PR should consider these points in their request (feel free to copy this checklist so you can fill it out yourself in the overall PR comment)