Skip to content
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

Merged
merged 21 commits into from
Aug 29, 2019

Conversation

assyadh
Copy link
Contributor

@assyadh assyadh commented Aug 17, 2019

On properties that are optionals, we now add a [JsiiOptional] attribute.

Ex:


[JsiiProperty(name: "optional1", typeJson: "{\"primitive\":\"string\"}", isOptional: true, isOverride: true)]
[System.Obsolete()]
[JsiiOptional]
public string Optional1
{
        get;
        set;
}

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:

Screen Shot 2019-08-16 at 5 13 35 PM (2)

Whit a required property passed with a null value:

Screen Shot 2019-08-16 at 5 14 12 PM (2)

With the required property correctly passed:

Screen Shot 2019-08-16 at 5 14 20 PM (2)

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.

@assyadh assyadh requested review from costleya and a team as code owners August 17, 2019 00:20
packages/jsii-dotnet-analyzers/LICENSE Outdated Show resolved Hide resolved

dotnet build --force -c Release ./src/Amazon.JSII.Analyzers

cp -f ./bin/Release/NuGet/*.nupkg .
Copy link
Contributor

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?

Copy link
Contributor Author

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?

packages/jsii-dotnet-analyzers/package.json Outdated Show resolved Hide resolved
internal static string VisualBasicDefaultExt = "vb";
internal static string TestProjectName = "TestProject";

#region Get Diagnostics
Copy link
Contributor

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... ❤️

Copy link
Contributor Author

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

@@ -0,0 +1,58 @@
param($installPath, $toolsPath, $package, $project)
Copy link
Contributor

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?

Copy link
Contributor Author

@assyadh assyadh Aug 19, 2019

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-analyzers/src/Directory.Build.props Outdated Show resolved Hide resolved
packages/jsii-dotnet-analyzers/src/NuGet.Metadata.props Outdated Show resolved Hide resolved
@assyadh
Copy link
Contributor Author

assyadh commented Aug 20, 2019

Made some changes on the analyzer rules following our meeting this morning.

See the Unit tests comments for some background :-)

@assyadh assyadh added the language/dotnet Related to .NET bindings (C#, F#, ...) label Aug 21, 2019
RomainMuller
RomainMuller previously approved these changes Aug 22, 2019
@mergify mergify bot dismissed RomainMuller’s stale review August 22, 2019 17:10

Pull request has been modified.

@assyadh
Copy link
Contributor Author

assyadh commented Aug 23, 2019

Travis build is blocked due to a transient issue:

Error downloading packages:
geronimo-jms-1.1.1-19.amzn2.noarch: [Errno 256] No more mirrors to try.

Copy link
Contributor

@costleya costleya left a 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?

@assyadh
Copy link
Contributor Author

assyadh commented Aug 23, 2019

[SuppressMessage("Usage", "JSII001")] is probably not quite what you want. Should we make it "Amazon.JSII" as the category?

It looks correct to me:

https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.codeanalysis.suppressmessageattribute.-ctor?view=netframework-4.8#System_Diagnostics_CodeAnalysis_SuppressMessageAttribute__ctor_System_String_System_String_

@RomainMuller RomainMuller self-assigned this Aug 26, 2019
costleya
costleya previously approved these changes Aug 26, 2019
@mergify mergify bot dismissed costleya’s stale review August 26, 2019 22:23

Pull request has been modified.

costleya
costleya previously approved these changes Aug 26, 2019
RomainMuller
RomainMuller previously approved these changes Aug 29, 2019
@mergify mergify bot dismissed RomainMuller’s stale review August 29, 2019 11:58

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Aug 29, 2019

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added pr/ready-to-merge This PR is ready to be merged. and removed pr/ready-to-merge This PR is ready to be merged. labels Aug 29, 2019
@mergify mergify bot added pr/ready-to-merge This PR is ready to be merged. and removed pr/ready-to-merge This PR is ready to be merged. labels Aug 29, 2019
@aws aws deleted a comment from mergify bot Aug 29, 2019
@aws aws deleted a comment from mergify bot Aug 29, 2019
@RomainMuller RomainMuller merged commit bece042 into master Aug 29, 2019
@RomainMuller RomainMuller deleted the hamzaad/optionals-roslyn branch August 29, 2019 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
language/dotnet Related to .NET bindings (C#, F#, ...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants