New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Prototype for discussion #10091
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Fixes #
Context
Changes Made
Testing
Notes