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

Enhance GenerateDocumentationAndConfigFiles tool to generate vNext globalconfig files #6258

Merged
merged 3 commits into from
Nov 16, 2022

Conversation

mavasani
Copy link
Contributor

Fixes #6247

Our generated globalconfig files that we ship in the analyzer NuGet packages and .NET SDK should likely include a vNext set of globalconfig files so that when the analysis level is set to latest or explicitly to vNext for upcoming .NET Release, we still find a proper mapped globalconfig file. This will avoid regressions such as #6245 in future.

Verified that locally built Microsoft.CodeAnalysis.NetAnalyzers package with this change includes globalconfig files specific to 8.0 release version.

…obalconfig files

Fixes dotnet#6247

Our generated globalconfig files that we ship in the analyzer NuGet packages and .NET SDK should likely include a vNext set of globalconfig files so that when the analysis level is set to latest or explicitly to vNext for upcoming .NET Release, we still find a proper mapped globalconfig file. This will avoid regressions such as dotnet#6245 in future.

Verified that locally built Microsoft.CodeAnalysis.NetAnalyzers package with this change includes globalconfig files specific to 8.0 release version.
@mavasani mavasani requested a review from a team as a code owner November 11, 2022 12:06
@codecov
Copy link

codecov bot commented Nov 11, 2022

Codecov Report

Merging #6258 (f88c0ae) into main (31373ce) will increase coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6258      +/-   ##
==========================================
+ Coverage   96.04%   96.06%   +0.01%     
==========================================
  Files        1360     1364       +4     
  Lines      312225   313686    +1461     
  Branches    10047    10125      +78     
==========================================
+ Hits       299889   301330    +1441     
- Misses       9928     9937       +9     
- Partials     2408     2419      +11     

@mavasani
Copy link
Contributor Author

@Evangelink @Youssef1313 can you please review? Thanks!

Copy link
Member

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

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

LGTM

src/Tools/GenerateDocumentationAndConfigFiles/Program.cs Outdated Show resolved Hide resolved
src/Tools/GenerateDocumentationAndConfigFiles/Program.cs Outdated Show resolved Hide resolved
@mavasani mavasani enabled auto-merge November 15, 2022 11:51
@mavasani mavasani disabled auto-merge November 15, 2022 11:51
Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

LGTM - few nitpicks you can ignore if you want.

Copy link
Member

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

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

@mavasani Took a look again at this, I'm not sure how this prevents the issue in the future.

The meaning of latest in analysis level is hardcoded to a specific version on dotnet/sdk

https://github.com/dotnet/sdk/blob/39fcce04a117339a8087060201b1f7797b6167e8/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.Analyzers.targets#L49

https://github.com/dotnet/sdk/blob/39fcce04a117339a8087060201b1f7797b6167e8/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.Analyzers.targets#L25

So the new globalconfig that contains unshipped will not be used.

@Youssef1313
Copy link
Member

Youssef1313 commented Nov 28, 2022

This is also currently confusing. I think 8 will enable more rules than latest (because latest is hard-coded to 7 in .NET 7 SDK)?

Please correct me if I'm misunderstanding something.

@mavasani
Copy link
Contributor Author

This is also currently confusing. I think 8 will enable more rules than latest (because latest is hard-coded to 7 in .NET 7 SDK)?

Please correct me if I'm misunderstanding something.

@Youssef1313 This change basically gives us a bit of leeway when the unshipped rules get moved to the AnalyzerReases.Shipped.txt file. latest setting is still meant to be associated with latest shipped rules, so it is working as expected. This change ensures that say once we create a new .NET 8 SDK branch and ship a preview release of the SDK, our tooling can find the matching globalconfig to map to the AnalysisLevel setting, even if we haven't moved our unshipped rules to AnalyzerReases.Shipped.txt because .NET 8 SDK wouldn't have shipped by then.

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.

Enhance GenerateDocumentationAndConfigFiles tool to generate vNext globalconfig files
3 participants