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

Enable CLS Compliance on Diagnostics Libraries #89214

Merged

Conversation

tarekgh
Copy link
Member

@tarekgh tarekgh commented Jul 19, 2023

This change will allow the consumers of these libraries not to be forced to set CLS Compliance off.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 19, 2023
@tarekgh tarekgh added area-System.Diagnostics and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 19, 2023
@tarekgh tarekgh added this to the 8.0.0 milestone Jul 19, 2023
@tarekgh tarekgh self-assigned this Jul 19, 2023
@noahfalk
Copy link
Member

I assume the affect of this change is that a CLSCompliant attribute at assembly scope changes from false to true (or maybe goes away if that is the default), and now other libraries that reference this one don't get compiler warnings? That sounds pretty good to me.

Are you aware of any scenario where the experience gets worse? I can't think of anything offhand.

@tarekgh
Copy link
Member Author

tarekgh commented Jul 19, 2023

I assume the affect of this change is that a CLSCompliant attribute at assembly scope changes from false to true (or maybe goes away if that is the default)

By default, if the CLSCompliant is not showing in the assembly, this assembly will NOT be compliant.
In our builds when specifying <CLSCompliant>false</CLSCompliant> in the csproj, this attribute will be fully removed, and the assembly will not tagged at all which make this assembly NOT compliant. But, if the csproj doesn't have <CLSCompliant>, the build manually adds <CLSCompliant>true</CLSCompliant> and making the assembly compliant. This means the change here is making the library CLS compliant. I confirmed that by checking the produced assemblies and ensuring the attribute is showing there with true value.

Are you aware of any scenario where the experience gets worse? I can't think of anything offhand.

I am not either, adding @ericstj @ViktorHofer if they can think in any broken scenario after this change. The opposite direction is the breaking one, I guess.

@ericstj
Copy link
Member

ericstj commented Jul 19, 2023

I'm not aware of anything breaking by adding CLSCompliant(true).

Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@tarekgh tarekgh merged commit a524267 into dotnet:main Jul 20, 2023
100 of 103 checks passed
@ghost ghost locked as resolved and limited conversation to collaborators Aug 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants