-
Notifications
You must be signed in to change notification settings - Fork 243
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
feat(dotnet): [JsiiOptional] attribute on properties that are optionals + Roslyn Analyzer #717
Conversation
|
||
dotnet build --force -c Release ./src/Amazon.JSII.Analyzers | ||
|
||
cp -f ./bin/Release/NuGet/*.nupkg . |
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 be copied to dist
instead?
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 based this file on the jsii-dotnet-runtime's one. It does not have a dist directory?
internal static string VisualBasicDefaultExt = "vb"; | ||
internal static string TestProjectName = "TestProject"; | ||
|
||
#region Get Diagnostics |
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.
Wish the runtime library was full of those, too... ❤️
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.
Pretty useful indeed. It can be tricky as everything has to be declared in a single file so that the diagnostic works (no references), but it works fine
packages/jsii-dotnet-analyzers/src/Amazon.JSII.Analyzers.UnitTests/JsiiOptionalAnalyzerTests.cs
Outdated
Show resolved
Hide resolved
packages/jsii-dotnet-analyzers/src/Amazon.JSII.Analyzers/JsiiOptionalAnalyzer.cs
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,58 @@ | |||
param($installPath, $toolsPath, $package, $project) |
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.
What's the use of this? How are people supposed to be using this?
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.
The only (ugly way) to use an analyzer inside an IDE is to reference its dll directly.
So, in order to avoid that, Microsoft gave Roslyn Analyzers vended by NuGet packages the option to be installed as analyzers, which really just adds this dll reference (not visible in the csproj though).
This is done by adding an install and uninstall scripts in the nupkg. When the analyzer is pulled from NuGet, it gets installed using that script in tools/install.ps1
I have tested this with Rider and Visual Studio.
See: https://docs.microsoft.com/fr-fr/nuget/guides/analyzers-conventions
And
https://blog.jetbrains.com/dotnet/2018/03/22/roslyn-analyzer-support-rider-2018-1-eap/
packages/jsii-dotnet-runtime/src/Amazon.JSII.Runtime/Deputy/JsiiOptionalAttribute.cs
Show resolved
Hide resolved
Made some changes on the analyzer rules following our meeting this morning. See the Unit tests comments for some background :-) |
Pull request has been modified.
Travis build is blocked due to a transient issue: Error downloading packages: |
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.
[SuppressMessage("Usage", "JSII001")] is probably not quite what you want. Should we make it "Amazon.JSII" as the category?
It looks correct to me: |
Pull request has been modified.
Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it! |
On properties that are optionals, we now add a [JsiiOptional] attribute.
Ex:
Also, This introduces a new package which needs to be pushed to NuGet: Amazon.Jsii.Analyzers.
It includes a Roslyn Analyzer which makes sure that inside nested props object initializations, all required properties are passed by the developer, and are not passed as null (syntax check only).
Example in Rider with a required property not passed:
Whit a required property passed with a null value:
With the required property correctly passed:
The idea here is to check inline initialization of data types within jsii classes.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.