-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 prototype proposal #9735
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.
Adding a few comments.
ab5267f
to
90bc89c
Compare
90bc89c
to
9314121
Compare
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.
Haven't looked at everything yet.
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.
Some more inline comments. Looking forward to having this in main!
src/Build/BuildCheck/Infrastructure/BuildCheckManagerProvider.cs
Outdated
Show resolved
Hide resolved
src/Build/BuildCheck/Acquisition/BuildCheckAcquisitionModule.cs
Outdated
Show resolved
Hide resolved
src/Build/BuildCheck/Infrastructure/BuildAnalyzerConfigurationInternal.cs
Show resolved
Hide resolved
Co-authored-by: Mariana Dematte <magarces@microsoft.com>
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.
Putting some questions/clarifications to get more details on the implementation and some small formatting suggestions.
src/Build/BuildCheck/Infrastructure/BuildAnalyzerConfigurationInternal.cs
Show resolved
Hide resolved
src/Build/BuildCheck/Infrastructure/BuildCheckConnectorLogger.cs
Outdated
Show resolved
Hide resolved
src/Build/BuildCheck/Infrastructure/BuildCheckManagerProvider.cs
Outdated
Show resolved
Hide resolved
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.
Checking one more time, every time have some clarifications,questions, sorry that I'm adding more comments in smaller chunks (some of them are regarding the testing basic coverage on some parts )
src/Build/BuildCheck/Infrastructure/BuildAnalyzerConfigurationInternal.cs
Show resolved
Hide resolved
Co-authored-by: Farhad Alizada <104755925+f-alizada@users.noreply.github.com>
… into exp/build-analyzers
Looks good to me! |
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 Thank you for addressing the comments:)
[celebrate] Yuliia Kovalova reacted to your message:
…________________________________
From: Jan Krivanek ***@***.***>
Sent: Monday, April 15, 2024 6:57:24 PM
To: dotnet/msbuild ***@***.***>
Cc: Yuliia Kovalova ***@***.***>; Review requested ***@***.***>
Subject: Re: [dotnet/msbuild] Analyzers prototype proposal (PR #9735)
Merged #9735<#9735> into main.
—
Reply to this email directly, view it on GitHub<#9735 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AWYM53RP6JZY6LRJGTPQPVTY5QPJJAVCNFSM6AAAAABDHA4YWOVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJSGQ3TKOBTG44DQNY>.
You are receiving this because your review was requested.Message ID: ***@***.***>
|
Context
Initial prototype of Analyzers/BuildCop project.
It's currently hidden behind a feature-flag in form of an explicit commandline argument
Design Doc
#9853