[spec] Indicate analyzer assets and respect analyzer filtering in project.assets.json#14455
[spec] Indicate analyzer assets and respect analyzer filtering in project.assets.json#14455kfikadu wants to merge 8 commits into
Conversation
nkolev92
left a comment
nkolev92
left a comment
There was a problem hiding this comment.
Apologies for the delay, great first write-up.
I added some comments about some specifics I'd love to see + some ideas for future possibilities.
kfikadu
changed the title
donnie-msft
left a comment
donnie-msft
left a comment
There was a problem hiding this comment.
Looks good!
Wondering what you think about renaming EnableIndicateAnalyzerAssets to be more concise, maybe IndicateAnalyzerAssets?
nkolev92
left a comment
nkolev92
left a comment
There was a problem hiding this comment.
This is looking great.
I may have left some feedback on the spec, but general direction seems fine.
Please go through the feedback and lmk if you have any questions.
Even if this spec is not fully merged yet, I think you can start working on the implementation
jeffkl
left a comment
jeffkl
left a comment
There was a problem hiding this comment.
My vote for the property name is RestoreEnableAnalyzerIncludeExcludeAssets
nkolev92
dismissed
their stale review
There's some feedback from folks around the analyzers folder structure.
|
@martinrrm - Could you please help understand next steps on this one? |
dotnet-policy-service
Bot
removed
the
Status:No recent activity
dotnet-policy-service
Bot
added
the
Status:No recent activity
martinrrm
requested review from
aortiz-msft,
baronfel,
donnie-msft,
jaredpar,
jeffkl,
kartheekp-ms,
martinrrm and
nkolev92
|
Updated the spec and created a PR for Client change NuGet/NuGet.Client#7464 and for SDK change dotnet/sdk#54646. |
| |Pattern|Description| | ||
| |---|---| | ||
| |`analyzers/**/{name}.dll`|Any analyzer assembly, at any depth under `analyzers/`| | ||
| |`analyzers/dotnet/{language}/{name}.dll`|Language-specific analyzer (`cs`, `vb`, `fs`)| | ||
| |`analyzers/dotnet/{compilerApiVersion}/{language}/{name}.dll`|Analyzer for a specific compiler API version (`roslynX.Y`)| |
There was a problem hiding this comment.
Does the first analyzers/**/{name}.dll pattern exclude the analyzers/dotnet/ directory? If not, what prevents analyzers/dotnet/fs/MyAnalyzer.dll from being used in a C# project?
There was a problem hiding this comment.
This change will add all of the files that start with analyzers/**/{name}.dll, reading this table again it suggests otherwise, but wanted to show that we are gathering language, compilerApiVersion and name from the path and using it in the metadata.
NuGet code won't prevent language selection; this list will be passed to the sdk and the sdk (which currently does) will decide which language to use.
| Excluded analyzers (for example via `PrivateAssets` or `ExcludeAssets`) are written as a `analyzers/.../_._` placeholder with no metadata. | ||
|
|
||
| Examples: | ||
| - `analyzers/dotnet/cs/MyAnalyzer.dll` (C# analyzer) |
There was a problem hiding this comment.
Everything is analyzers/dotnet/ right?
Do we automatically match dotnet or is somehow configurable?
11 participants
Fixes: #6279
Rendered MD