-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Move RDG omission to ResolveTargetingPackAssets target #31265
Conversation
Thanks for your PR, @captainsafia. |
@dsplaisted Thoughts on this? Would be great to get it in for preview3. |
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.
Should this work on analyzers coming from NuGet packages too? I think I remember that we discussed that it should, but it looks like in this case it will only work on analyzers from targeting packs.
Otherwise, looks good.
<AnalyzerToRemove Include="@(OffByDefaultAnalyzer)" | ||
Path="%(Analyzer.Identity)" | ||
Condition="'%(Analyzer.Filename)' == '@(OffByDefaultAnalyzer)'" /> |
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.
There are tests covering this, right? The join is a bit tricky and I probably wouldn't get it right on the first try.
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.
FWIW here's the current logic for handling analyzers in NuGet packages in the runtime (to provide the generator to folks using older SDKs) - https://github.com/dotnet/runtime/blob/1e601c5101e1f0573cd03560615aa8f6cb38fa64/eng/packaging.targets#L196-L229
Is there any concern around these targets conflicting with each other?
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.
There are tests covering this, right? The join is a bit tricky and I probably wouldn't get it right on the first try.
Yep, there are pre-existing tests for the behavior in this test class which is how I validated it worked (definitely not on the first try though 🤪).
Should this work on analyzers coming from NuGet packages too?
At least for this PR, I was focused on the case for analyzers derived from targeting packs as described here (#30872 (comment)).
Is there any concern around these targets conflicting with each other?
It doesn't seem like they would conflict, especially because the config generator has not yet been added to the list of OffByDefaultAnalyzer
s (assuming this PR is merged).
I'm not sure if it is worthwhile to move the targets that are currently in the runtime to the SDK.
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.
I see, looks good.
FYI @marcpopMSFT, this PR should go into preview 3 if possible. |
Is this good to merge now? There's one reaction in ASP.NET we'd like to get in for preview3 after this. We want to rename our generator assembly from Microsoft.AspNetCore.Http.Generators => Microsoft.AspNetCore.Http.Generator once this change is in. |
Related to #30872.
Centralize the configuration for off-by-default analyzers/generators into a new build target that runs after
ResolveTargetingPackAssets
. The idea is that we roll out new generators that need to be off-by-default, we would add new items to theOffByDefaultAnalyzer
item group.