Skip to content

Convert .ruleset to .globalconfig#49181

Merged
sharwell merged 4 commits intodotnet:masterfrom
sharwell:globalconfig
Nov 5, 2020
Merged

Convert .ruleset to .globalconfig#49181
sharwell merged 4 commits intodotnet:masterfrom
sharwell:globalconfig

Conversation

@sharwell
Copy link
Contributor

@sharwell sharwell commented Nov 4, 2020

No description provided.

<!-- Always include Common.globalconfig -->
<EditorConfigFiles Include="$(RepositoryEngineeringDir)config\rulesets\Common.globalconfig" />
<!-- Include Shipping.globalconfig for shipping projects -->
<EditorConfigFiles Condition="'$(IsShipping)' == 'true'" Include="$(RepositoryEngineeringDir)config\rulesets\Shipping.globalconfig" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Tagging @chsienki - I believe including these as GlobalAnalyzerConfigFiles should work and make it more clear that these are global analyzer configs, not editorconfigs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two problems with this approach:

  1. This repository currently uses a compiler build that does not support this property

  2. The conversion of GlobalAnalyzerConfigFiles to EditorConfigFiles is conditioned on $(DiscoverGlobalAnalyzerConfigFiles):

    <EditorConfigFiles Include="@(GlobalAnalyzerConfigFiles->Exists())" Condition="'$(DiscoverGlobalAnalyzerConfigFiles)' != 'false'" />

    We would need to remove this condition (and wait for it to apply to the SDK used for this repository) before GlobalAnalyzerConfigFiles becomes a viable alternative to EditorConfigFiles.

Copy link
Contributor

Choose a reason for hiding this comment

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

Confirmed the property GlobalAnalyzerConfigFiles works as expected: ClassLibrary58.zip

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirmed the property GlobalAnalyzerConfigFiles works as expected

It might work in newer SDKs, but it doesn't work in the version of the SDK and/or compiler currently specified for Roslyn command line builds.

It also doesn't address the fact that setting unrelated property DiscoverGlobalAnalyzerConfigFiles currently disables the manual inclusion of global analyzer config files.

Copy link
Contributor

Choose a reason for hiding this comment

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

DiscoverGlobalAnalyzerConfigFiles is not defaulted anywhere as per my knowledge, so the condition should be true by default and current logic should work.

Copy link
Contributor

Choose a reason for hiding this comment

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

It also doesn't address the fact that setting unrelated property DiscoverGlobalAnalyzerConfigFiles currently disables the manual inclusion of global analyzer config files.

Yes, that seems reasonable - @chsienki I can file a tracking bug for you if you agree.

@@ -0,0 +1,99 @@
is_global = true

dotnet_diagnostic.CA1067.severity = warning
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: I did not validate that each of the entries in the global configs map to the entries in current rulesets.

@@ -1,95 +0,0 @@
<?xml version="1.0" encoding="utf-8"?>
<RuleSet Name="Common diagnostic rules to run during build for all shipping Roslyn projects" Description="This file contains diagnostic settings used by all Roslyn projects. Projects that need specific settings should have their own rule set files that Include this one, and then make the necessary adjustments." ToolsVersion="14.0">
Copy link
Member

Choose a reason for hiding this comment

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

image

@sharwell sharwell merged commit ff986c1 into dotnet:master Nov 5, 2020
@ghost ghost added this to the Next milestone Nov 5, 2020
@sharwell sharwell deleted the globalconfig branch November 5, 2020 17:33
@allisonchou allisonchou modified the milestones: Next, 16.9.P2 Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants