The Wayback Machine - https://web.archive.org/web/20211215150451/https://github.com/dotnet/aspnetcore/pull/30138
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

Jump to bottom

Use NetAnalyzers from SDK instead of FxCopAnalyzers #30138

Merged
merged 26 commits into from Feb 19, 2021
Merged

Use NetAnalyzers from SDK instead of FxCopAnalyzers #30138

merged 26 commits into from Feb 19, 2021

Conversation

@Youssef1313
Copy link
Contributor

@Youssef1313 Youssef1313 commented Feb 12, 2021 •
edited
Loading

Fixes #30099

  • Replaces FxCopAnalyzers package references with NetAnalyzers. (Used from .NET SDK instead of package)
  • Deletes rulset files
  • Migrate rulesets to editorconfigs

@sharwell Can you review?
@Youssef1313 Youssef1313 requested a review from as a code owner Feb 12, 2021
Youssef1313
Youssef1313 reviewed Feb 12, 2021
View changes
eng/targets/CSharp.Common.props Outdated Show resolved Hide resolved
Loading
@Youssef1313 Youssef1313 changed the title Use NetAnalyzers WIP: Use NetAnalyzers Feb 12, 2021
@Youssef1313 Youssef1313 requested a review from dougbu as a code owner Feb 12, 2021
@Youssef1313 Youssef1313 force-pushed the migrate-analyzers branch from caace89 to f8a3736 Feb 12, 2021
@Youssef1313 Youssef1313 changed the title WIP: Use NetAnalyzers WIP: Use NetAnalyzers from SDK instead of FxCopAnalyzers Feb 12, 2021
@Youssef1313 Youssef1313 force-pushed the migrate-analyzers branch from f8a3736 to 09068aa Feb 12, 2021
@Youssef1313 Youssef1313 force-pushed the migrate-analyzers branch from 4a7aa37 to ea93e93 Feb 12, 2021
@Youssef1313
Copy link
Contributor Author

@Youssef1313 Youssef1313 commented Feb 12, 2021 •
edited
Loading

There CA1310 violations like:

aspnetcore/src/Razor/Microsoft.AspNetCore.Razor.Language/src/Components/ComponentBindLoweringPass.cs

Line 499 in c925f99

if (!attributeName.StartsWith("bind-"))

Do you want me to pass a StringComparison.Ordinal, or StringComparison.Invariant, or disable the rule?

I'm not sure why the main branch doesn't fail with this error. It's already enabled without my changes 😄

Loading

@Youssef1313 Youssef1313 mentioned this pull request Feb 12, 2021
Closed
@Pilchie Pilchie self-assigned this Feb 12, 2021
Pilchie
Pilchie reviewed Feb 12, 2021
View changes
Copy link
Member

@Pilchie Pilchie left a comment

@pranavkm or @NTaylorMullen any ideas on why it doesn't already fail?

I definitely prefer Ordinal comparisons whenever possible, especially for language keyword comparisons.

Loading

src/ProjectTemplates/Web.Spa.ProjectTemplates/content/Angular-CSharp/ClientApp/.editorconfig Outdated Show resolved Hide resolved
Loading
src/Razor/Microsoft.AspNetCore.Razor.Language/src/SourceSpan.cs
{
return false;
}

return left.Equals(right);
Copy link
Member

@Pilchie Pilchie Feb 12, 2021

Choose a reason for hiding this comment

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

Note - safe because SourceSpan is a struct, so can't be null, and will never be ReferenceEquals.

Loading

@Pilchie
Copy link
Member

@Pilchie Pilchie commented Feb 12, 2021

Looks good so far - let me know when you're done.

Loading

dougbu
dougbu reviewed Feb 12, 2021
View changes
.editorconfig Show resolved Hide resolved
Loading
sharwell
sharwell reviewed Feb 14, 2021
View changes
.editorconfig Outdated
dotnet_diagnostic.CA1305.severity = error
dotnet_diagnostic.CA1310.severity = error
# Temporarily disabling, waiting on PR feedback to re-enable
dotnet_diagnostic.CA1310.severity = none
Copy link
Member

@sharwell sharwell Feb 14, 2021

Choose a reason for hiding this comment

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

💡 Setting diagnostic severity is more efficient if it's done in a globalconfig instead of editorconfig. See dotnet/razor-tooling#3087 for an example of how I converted aspnetcore-tooling from rulesets to globalconfig.

Loading

Youssef1313
Youssef1313 reviewed Feb 14, 2021
View changes
src/Mvc/Directory.Build.props Show resolved Hide resolved
Loading
@Youssef1313 Youssef1313 force-pushed the migrate-analyzers branch from 8c6b64f to 2dd807e Feb 14, 2021
@Youssef1313 Youssef1313 force-pushed the migrate-analyzers branch from cb0923c to 05f1003 Feb 14, 2021
@Youssef1313 Youssef1313 force-pushed the migrate-analyzers branch from 5643992 to 2179a45 Feb 14, 2021
@Youssef1313
Copy link
Contributor Author

@Youssef1313 Youssef1313 commented Feb 14, 2021 •
edited
Loading

There are some violations of CA1305 from https://github.com/aspnet/MessagePack-CSharp.git which is imported through gitmodules. I'll move this specific rule to .editorconfig instead of .globalconfig as a workaround.

Loading

@Youssef1313 Youssef1313 changed the title WIP: Use NetAnalyzers from SDK instead of FxCopAnalyzers Use NetAnalyzers from SDK instead of FxCopAnalyzers Feb 15, 2021
@Youssef1313
Copy link
Contributor Author

@Youssef1313 Youssef1313 commented Feb 15, 2021

This should be ready for another look.

Loading

@Youssef1313 Youssef1313 requested review from Pilchie, dougbu and sharwell Feb 15, 2021
Pilchie
Pilchie approved these changes Feb 15, 2021
View changes
Copy link
Member

@Pilchie Pilchie left a comment

This looks good to me, but I'd like @pranavkm or @NTaylorMullen to make sure they agree with me about the Ordinal comparisons in the Razor compiler before we merge.

Loading

.editorconfig Show resolved Hide resolved
Loading
src/Testing/src/xunit/SkipOnHelixAttribute.cs Outdated Show resolved Hide resolved
Loading
pranavkm
pranavkm approved these changes Feb 19, 2021
View changes
Copy link
Contributor

@pranavkm pranavkm left a comment

Thanks, this is absolutely great!

Loading

src/Testing/src/xunit/SkipOnHelixAttribute.cs
@@ -52,7 +52,7 @@ private bool ShouldSkip()

var targetQueue = GetTargetHelixQueue().ToLowerInvariant();

if (Queues.Contains("All.OSX") && targetQueue.StartsWith("osx"))
if (Queues.Contains("All.OSX") && targetQueue.StartsWith("osx", StringComparison.OrdinalIgnoreCase))
Copy link
Contributor

@pranavkm pranavkm Feb 19, 2021

Choose a reason for hiding this comment

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

Suggested change
if (Queues.Contains("All.OSX") && targetQueue.StartsWith("osx", StringComparison.OrdinalIgnoreCase))
if (Queues.Contains("All.OSX") && targetQueue.StartsWith("osx", StringComparison.Ordinal))

That would be the equivalent to the previous check

Loading

Copy link
Contributor

@pranavkm pranavkm Feb 19, 2021

Choose a reason for hiding this comment

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

I'll let this be. This is test-only code so it doesn't make a difference.

Loading

Copy link
Member

@Pilchie Pilchie Feb 19, 2021

Choose a reason for hiding this comment

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

@HaoK actually asked to change this to OrdinalIgnoreCase as part of this review.

Loading

@pranavkm pranavkm merged commit 350ea5b into dotnet:main Feb 19, 2021
25 checks passed
Loading
@Youssef1313 Youssef1313 deleted the migrate-analyzers branch Feb 19, 2021
dougbu pushed a commit to dougbu/razor-compiler that referenced this issue Nov 17, 2021
@Youssef1313
Use NetAnalyzers from SDK instead of FxCopAnalyzers (dotnet/aspnetcor…
d651a81 
…e#30138)

* Use analyzers from .NET SDK


Commit migrated from dotnet/aspnetcore@350ea5b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pranavkm pranavkm

@Pilchie Pilchie

@HaoK HaoK

@halter73 halter73

@javiercn javiercn

@jkotalik jkotalik

@Tratcher Tratcher

@dougbu dougbu

@sharwell sharwell

Assignees

@Pilchie Pilchie

Labels
area-infrastructure community-contribution
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Migrate from FxCopAnalyzers to NetAnalyzers
7 participants
@Youssef1313 @Pilchie @pranavkm @sharwell @dougbu @HaoK @javiercn