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

Move to latest .NET8 pre-release version of NetAnalyzers package for dogfooding #6380

Merged
merged 29 commits into from
Dec 22, 2022

Conversation

mavasani
Copy link
Contributor

Recommended to be reviewed commit by commit

This PR also fixes most of the new violations.

#6379 tracks auditing certain suppressed violations so these are either fixed and suppressions removed or we decide that the suppressions are desirable.

…erride equals and operator equals on value types
…tions if appropriate): For improved performance, use the LoggerMessage delegates instead of calling 'LoggerExtensions.LogInformation(ILogger, string?, params object?[])'
…lection) to suggestion. TODO: File an issue to fix or suppress CA1851 violations in the repo and revert this downgrade.
…ndexer access guarded by a 'ContainsKey' check to avoid double lookup
…ary between calls to 'LoggerExtensions.LogInformation(ILogger, string?, params object?[])'
…ns(char)' instead of 'string.Contains(string)' when searching for a single character
@mavasani mavasani requested a review from a team as a code owner December 22, 2022 06:04
@mavasani
Copy link
Contributor Author

@Youssef1313 for review

@mavasani mavasani mentioned this pull request Dec 22, 2022
@codecov
Copy link

codecov bot commented Dec 22, 2022

Codecov Report

Merging #6380 (cd01f06) into main (2b5a3b2) will increase coverage by 0.00%.
The diff coverage is 77.94%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6380   +/-   ##
=======================================
  Coverage   96.10%   96.10%           
=======================================
  Files        1361     1361           
  Lines      316050   316037   -13     
  Branches    10186    10193    +7     
=======================================
- Hits       303755   303743   -12     
+ Misses       9867     9862    -5     
- Partials     2428     2432    +4     

@@ -47,7 +47,7 @@
<MicrosoftCodeAnalysisForRoslynDiagnosticsAnalyzersVersion>3.7.0</MicrosoftCodeAnalysisForRoslynDiagnosticsAnalyzersVersion>
<MicrosoftCodeAnalysisVersionForTests>4.4.0-2.22416.9</MicrosoftCodeAnalysisVersionForTests>
<DogfoodAnalyzersVersion>3.3.4-beta1.22418.3</DogfoodAnalyzersVersion>
<DogfoodNetAnalyzersVersion>7.0.0</DogfoodNetAnalyzersVersion>
<DogfoodNetAnalyzersVersion>8.0.0-preview1.22621.6</DogfoodNetAnalyzersVersion>
Copy link
Member

Choose a reason for hiding this comment

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

So is 7.0.0 broken? If that's the case, I think it's important to fix that and release on NuGet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, AnalysisLevel/AnalysisMode regression is first fixed in 7.0.101. We definitely need to release the corresponding NuGet package on NuGet.org. Tagging @jmarolf

Copy link
Member

Choose a reason for hiding this comment

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

@mavasani The 7.0.0 tag is pointing to e26a04c, which is supposed to have the fix:

image

Is the tag pointing to incorrect commit?

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 am guessing so

Comment on lines +12 to +13
protected override bool IsExpressionOfForEachStatement(SyntaxNode syntax)
=> syntax.Parent is ForEachStatementSyntax forEachStatementSyntax && forEachStatementSyntax.Expression.Equals(syntax);
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated to the PR, but it looks like we have a false negative here. Guessing this should be CommonForEachStatementSyntax

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

@mavasani
Copy link
Contributor Author

Thank you @Youssef1313

@mavasani mavasani merged commit 460a0e5 into dotnet:main Dec 22, 2022
@mavasani mavasani deleted the UpgradeNetAnalyzers branch December 22, 2022 07:53
@github-actions github-actions bot added this to the vNext milestone Dec 22, 2022
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.

2 participants