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.

Sign up for GitHub

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

Jump to bottom

Fix various IDE and CA diags raised on master #4555

Merged
merged 14 commits into from
Dec 20, 2020
Merged

Fix various IDE and CA diags raised on master #4555

merged 14 commits into from
Dec 20, 2020

Conversation

Evangelink
Copy link
Member

Fix various IDE and CA rules raised on the repo.

@Evangelink Evangelink requested a review from a team as a code owner December 10, 2020 21:58
@codecov
Copy link

codecov bot commented Dec 11, 2020 •
edited
Loading

Codecov Report

Merging #4555 (1e6787b) into master (9b18190) will decrease coverage by 0.00%.
The diff coverage is 70.51%.

@@            Coverage Diff             @@
##           master    #4555      +/-   ##
==========================================
- Coverage   95.77%   95.77%   -0.01%     
==========================================
  Files        1193     1193              
  Lines      269069   269067       -2     
  Branches    16243    16245       +2     
==========================================
- Hits       257704   257698       -6     
+ Misses       9274     9260      -14     
- Partials     2091     2109      +18

@Evangelink Evangelink requested a review from mavasani December 11, 2020 21:13
mavasani
mavasani reviewed Dec 11, 2020
View reviewed changes
Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

Couple main points:

  1. I would like to see the change in the PR to bump up our CodeStyleAnalyzerVersion so the analyzer violations fixed here become build warnings and future violations fail build.
  2. We would want the 2.9.x change to go in and merge with master before taking this PR into master

@mavasani
Copy link
Contributor

Additionally, please make sure any in-source suppressions or editorconfig settings for these 4 rules IDs are removed from the repo (where approapriate).

@Evangelink Evangelink changed the title Fix IDE0100, IDE0090, CA1725 and CA1305 [WIP] Fix IDE0100, IDE0090, CA1725 and CA1305 Dec 14, 2020
@mavasani
Copy link
Contributor

This should be unblocked once #4573 is merged.

@Evangelink Evangelink changed the title [WIP] Fix IDE0100, IDE0090, CA1725 and CA1305 Fix various IDE and CA diags raised on master Dec 17, 2020
@Evangelink Evangelink requested a review from mavasani December 17, 2020 10:10
@Evangelink
Copy link
Member Author

Looks like there is an issue with tests on Ubuntu, this may be linked to #4586 and #4587.

@mavasani
Copy link
Contributor

I am still confused why these CA and IDE diagnostics are not being flagged on build prior to this PR. Your recent PR to move to CodeStyle layer to 3.8.0 should ensure these are enabled on build and violations should cause build failures:

roslyn-analyzers/eng/Versions.props

Lines 27 to 29 in b37ca50

<MicrosoftNETCoreCompilersPackageVersion>3.8.0</MicrosoftNETCoreCompilersPackageVersion>
<MicrosoftNetCompilersToolsetVersion>$(MicrosoftNETCoreCompilersPackageVersion)</MicrosoftNetCompilersToolsetVersion>
<CodeStyleAnalyersVersion>$(MicrosoftNETCoreCompilersPackageVersion)</CodeStyleAnalyersVersion>

I will investigate further.

@mavasani
Copy link
Contributor

mavasani commented Dec 17, 2020 •
edited
Loading

@Evangelink Seems like there are lot of IDE and CA violations even in 2.9.x branch which are warnings in IDE, but still not failing build. I will fix the 2.9.x branch first and handle the violations in master when the PR merges into master.

@mavasani
Copy link
Contributor

mavasani commented Dec 17, 2020 •
edited
Loading

Seems like there are lot of IDE and CA violations even in 2.9.x branch which are warnings in IDE, but still not failing build.

The IDE0090 warnings not failing build seem to be in 3.9.x beta CodeStyle NuGet packages, not in the 3.8.0 ones, hence these are not failing build. I will try to bump the version even higher in 2.9.x branch and try to get these fixed as part of the update.

mavasani
mavasani reviewed Dec 17, 2020
View reviewed changes
src/Utilities/Compiler/Options/SymbolNamesWithValueOption.cs Outdated
@@ -337,7 +338,9 @@ static string GetDeclarationId(ISymbol symbol)
}
}

#pragma warning disable CA1024 // Use properties where appropriate
Copy link
Contributor

Choose a reason for hiding this comment

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

This diagnostic does not seem to be a build warning. We normally avoid adding source suppressions for IDE-suggestions.

mavasani
mavasani reviewed Dec 17, 2020
View reviewed changes
Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Have a question about a newly added field + request to revert all suppressions added for non-warning diagnostics.

@mavasani mavasani mentioned this pull request Dec 17, 2020
Closed
@Evangelink Evangelink requested a review from mavasani December 19, 2020 21:56
@mavasani mavasani merged commit 0dfc0c2 into dotnet:master Dec 20, 2020
@Evangelink Evangelink deleted the refactorings branch December 20, 2020 10:48
@Evangelink Evangelink mentioned this pull request Dec 31, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dotpaul dotpaul dotpaul left review comments

@mavasani mavasani mavasani approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Evangelink @mavasani @dotpaul