Conversation
JanKrivanek
left a comment
JanKrivanek
left a comment
There was a problem hiding this comment.
Looks good!
I need to look on #10018 and have a second look on this to form a firm opinion..
| if (wasBuildCheckEnabled == true) | ||
| { | ||
| buildManager.ConfigureBuildCheck(true); | ||
| } |
There was a problem hiding this comment.
this feels that it might not need to be conditional - if the buildcheck was not enabled, then the _buildCheckManagerForEnabledState is anyways the null pattern manager - and hence setting doesn't hurt
There was a problem hiding this comment.
Correct. If we go this way, I plan to clean it a bit, so far I was mostly focusing on describing the approach
| private IBuildCheckManager _buildCheckManager; | ||
| private readonly IBuildCheckManager _buildCheckManagerForEnabledState; |
There was a problem hiding this comment.
Btw. I definitely like this swapping approach.
I'm not 100% sure whether we want to have the infra dictate if restore is happening or have the other approach - storing the flag in BuildEvaluationStartedEventArg (as then the handling might be more unified for the replay case).
But even if we stay with BuildEvaluationStartedEventArg case - I'd like to adopt this swapping strategy
2 participants
Fixes #
Context
Changes Made
Testing
Notes