Add Roslyn Analyzer project #349
Conversation
|
|
||
| {{type}} MyCarterModule : ICarterModule | ||
| { | ||
| internal {|#0:MyCarterModule|}(string s) {} |
There was a problem hiding this comment.
Out of interest what is the \#0: stuff?
There was a problem hiding this comment.
This is Roslyn's markup syntax, allowing to specify where a diagnostic is supposed to be emitted, without having to specify the exact line and column span at which the diagnostic occurs in the DiagnosticResult. Hence the .WithLocation(0);. If the analyzer emitted diagnostics at multiple locations, I would use {|#1: and then call .WithLocation(1);.
| <Project Sdk="Microsoft.NET.Sdk"> | ||
|
|
||
| <PropertyGroup> | ||
| <TargetFramework>netstandard2.0</TargetFramework> |
There was a problem hiding this comment.
It could be, however I'd advise against it if you want maximum compatibility. Roslyn components are supposed to target netstandard2.0. I've noticed targeting higher versions works too, however not for everybody. Rider always works fine, it might just not work for some Visual Studio users. For most Visual Studio users it will also be fine.
Considering you want to ship this with the Carter package, I say go for it and if we notice that the analyzer does not kick in for the majority of users, we can still separate this into a separate package. I will make the necessary adjustments.
| @@ -0,0 +1,35 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
There was a problem hiding this comment.
I'd like to ship the analyser with Carter so should Carter reference this project that is then embedded in Carter's nuget package?
There was a problem hiding this comment.
I will look into this, I'm not sure if analyzers work as transitive dependencies.
|
I managed to pack the analyzer with the Carter package and still keep the analyzer project Also since the analyzer is now also added to the Carter project, we got a hit. The |
|
Looking good - excuse my ignorance but any reason why we can't delete the analyzer project and just put the analyzer classes inside Carter? |
|
No worries! We can do that, however the analyzer might not work for a very small amount of users due to the targeting of |
| </PropertyGroup> | ||
| <ItemGroup> | ||
| <None Include="..\..\media\carterlogo.png" Pack="true" PackagePath="\" /> | ||
| <None Include="..\..\README.md" Pack="true" PackagePath="\"/> | ||
| <None Include="$(OutputPath)\$(AssemblyName).dll" Pack="true" PackagePath="analyzers/dotnet/cs" Visible="false" /> |
There was a problem hiding this comment.
Excuse my ignorance again - what does this do?
There was a problem hiding this comment.
Please don't hold back with questions – it's important you understand the code I added :)
To answer your question: It's not enough to add the code to the project. To make analyzers work inside a NuGet package, they must be placed in a specific directory inside the package, in this case the analyzers/dotnet/cs directory, since it is a C# analyzer.
| @@ -89,7 +89,9 @@ private static Assembly SafeLoadAssembly(AssemblyName assemblyName) | |||
| { | |||
| try | |||
| { | |||
| #pragma warning disable RS1035 | |||
There was a problem hiding this comment.
Should we add this to the NoWarn csproj list?
There was a problem hiding this comment.
No, since this warning would be valid if DependencyContextAssemblyCatalog were an analyzer. Since it is not, the warning can be suppressed. Just one small caveat of not splitting up library code and analyzers.
| @@ -88,12 +88,14 @@ public static async Task BindAndSaveFile(this HttpRequest request, string saveLo | |||
|
|
|||
| private static async Task SaveFileInternal(IFormFile file, string saveLocation, string fileName = "") | |||
| { | |||
| #pragma warning disable RS1035 | |||
There was a problem hiding this comment.
Should we add this to the NoWarn csproj list?
|
If I put in a ctor dependency in the Carter.Sample project that has a project ref to Carter will I still get a warning? |
|
Yes, you will, however the project reference must look like this to include analyzer behavior:
|
|
Should rename it to |
|
We could, however I wouldn't recommend it. The guidelines for diagnostic IDs recommend using the project name + a number. If you decide you want another analyzer to warn about something else in the future, you'd need to distinguish it from this one, e.g. by naming it |
|
Can you rebase, I've added some permissions to the actions workflow file :) |
|
Actually GH has a button to do it :) |
|
Thanks for this!! Appreciate it 👍 👍 |
|
Happy to be of assistance! Ping me if you want any other rules implemented 😄 |
As promised, I have created a Roslyn Analyzer which emits a warning when a type implementing
ICarterModulehas a constructor with parameters. Feel free to look over the test cases and see if you agree with the behavior.