Skip to content

Apply code fixes from recently added analyzers#36097

Merged
pgovind merged 6 commits intodotnet:masterfrom
pgovind:StringIndexOfAnalyzer
Jun 3, 2020
Merged

Apply code fixes from recently added analyzers#36097
pgovind merged 6 commits intodotnet:masterfrom
pgovind:StringIndexOfAnalyzer

Conversation

@pgovind
Copy link

@pgovind pgovind commented May 8, 2020

Code-fixes triggered by dotnet/roslyn-analyzers#3610 and #33786

I've been careful to make changes only on the projects that target netstandard2.1+ and add the analyzer to the NoWarn tags for the ones that also target 2.0.

@pgovind pgovind requested a review from bartonjs May 8, 2020 03:23
@stephentoub stephentoub changed the title Code-fixes Apply code fixes from recently added analyzers May 8, 2020
@pgovind pgovind force-pushed the StringIndexOfAnalyzer branch from da39c94 to c8e7440 Compare May 11, 2020 19:23
@pgovind
Copy link
Author

pgovind commented May 17, 2020

Friendly bump. Can I get a stamp on this please?

<AssemblyName>System.Net.WebSockets</AssemblyName>
<AllowUnsafeBlocks>True</AllowUnsafeBlocks>
<NoWarn>$(NoWarn);CS1573</NoWarn>
<NoWarn>$(NoWarn);CS1573;CA2249</NoWarn>
Copy link
Member

Choose a reason for hiding this comment

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

Why was this rule disabled here?

Copy link
Author

Choose a reason for hiding this comment

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

Testing with the rule enabled

<AssemblyName>System.Data.Common</AssemblyName>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<NoWarn>$(NoWarn);CS1573</NoWarn>
<NoWarn>$(NoWarn);CS1573;CA2249</NoWarn>
Copy link
Member

Choose a reason for hiding this comment

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

Why was this rule disabled here? (Questioning it because this project only targets netcoreapp)

Copy link
Author

@pgovind pgovind May 20, 2020

Choose a reason for hiding this comment

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

I honestly had the same questions. The CI build failed here without suppressing CA2249 even though the target says netcoreapp. I just assumed our build infra was also building this for another config somehow. Actually, let's test it again now

<WarningLevel>4</WarningLevel>
<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
<NoWarn>$(NoWarn),0419,0649</NoWarn>
<NoWarn>$(NoWarn),0419,0649,CA2249,CA1830</NoWarn>
Copy link
Member

Choose a reason for hiding this comment

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

Why are these disabled for corelib?

Copy link
Author

Choose a reason for hiding this comment

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

CA2249 is "Prefer string.Contains over string.IndexOf" and CA1830 is "Prefer typed StringBuilder overloads". Both these APIs are defined in corelib right? So they'd trigger the analyzer but there's nothing to do other than suppress them. I could make the suppression local if you'd prefer that

@pgovind pgovind merged commit 6b6f520 into dotnet:master Jun 3, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
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