Skip to content
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

Analyzers acquisition experience #9977

Conversation

YuliiaKovalova
Copy link
Member

@YuliiaKovalova YuliiaKovalova commented Apr 8, 2024

Context

This PR implements acquisition flow for custom analyzers.

The process can be described the next way:
A Custom Analyzer library contains a file Company.AnalyzerTemplate.props.
image

If this analyzer is referenced from any solution, MSBuild invokes intrinsic function RegisterAnalyzer during the evaluation. It emits BuildCheckAcquisitionEventArgs if the parsed path to custom analyzer is valid.
This event is handled by BuildCheckConnectorLogger: it passes the info to IBuildCheckManager.ProcessAnalyzerAcquisition.
image

Tests

Manual and unit tests.
I plan to cover e2e scenarios once MSBuild.Build package with BuildCheck infrastructure is available for public reference (it is needed for custom templates).

@YuliiaKovalova YuliiaKovalova force-pushed the dev/ykovalova/analyzers_aquisition_experience branch from 3b41327 to 53a7c8b Compare April 8, 2024 11:16
@YuliiaKovalova YuliiaKovalova marked this pull request as ready for review April 9, 2024 11:15
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will need to do deeper pass for approve/request changes result - but overall looks good.

It feels this deserves a section in documentation

src/Build/Evaluation/Expander.cs Show resolved Hide resolved
src/Build/Resources/Strings.resx Outdated Show resolved Hide resolved
src/Build/Evaluation/Expander.cs Outdated Show resolved Hide resolved
src/Build/Resources/Strings.resx Outdated Show resolved Hide resolved
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

I added couple comments - the double iteration in acquisition module is reason for requesting changes, but otherwise it looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants