-
Notifications
You must be signed in to change notification settings - Fork 467
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
Address CA1862 cases to diagnose and to fix and improve messages #6928
Conversation
...re/Microsoft.NetCore.Analyzers/Performance/RecommendCaseInsensitiveStringComparison.Fixer.cs
Show resolved
Hide resolved
...crosoft.NetCore.Analyzers/Performance/CSharpRecommendCaseInsensitiveStringComparisonFixer.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @carlossanlop for getting this done!
Will be good if @buyaa-n can take a look too.
...crosoft.NetCore.Analyzers/Performance/RecommendCaseInsensitiveStringComparison.Base.Tests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
There are a lot of failures in the CI that Visual Studio is not showing. I was able to see them in my local machine by running the tests in the console using Very frustrating. Between this blind spot and the Roslyn suggestions that show up in CI but not in the IDE, the experience has been painful with Visual Studio. |
OK I figured out the problem. The tests that are failing are the ones where the arguments are named and they are passed unordered.
The C# tests are currently failing because I thought both languages restored the order to the original one, but C# needs to respect the order passed by the user. I will split the tests and will provide different expected data for VB and in C#. |
Codecov Report
@@ Coverage Diff @@
## main #6928 +/- ##
==========================================
- Coverage 96.40% 96.39% -0.01%
==========================================
Files 1396 1402 +6
Lines 330443 332159 +1716
Branches 10891 11061 +170
==========================================
+ Hits 318564 320195 +1631
- Misses 9152 9195 +43
- Partials 2727 2769 +42 |
Finally, green CI! 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some comment mostly for messaging, overall LGTM, thank you!
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx
Outdated
Show resolved
Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx
Show resolved
Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx
Outdated
Show resolved
Hide resolved
...re/Microsoft.NetCore.Analyzers/Performance/RecommendCaseInsensitiveStringComparison.Fixer.cs
Outdated
Show resolved
Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx
Show resolved
Hide resolved
/backport to release/8.0 |
Started backporting to release/8.0: https://github.com/dotnet/roslyn-analyzers/actions/runs/6205295902 |
@carlossanlop an error occurred while backporting to release/8.0, please check the run log for details! Error: The specified backport target branch release/8.0 wasn't found in the repo. |
/backport to release/8.0.1xx |
Started backporting to release/8.0.1xx: https://github.com/dotnet/roslyn-analyzers/actions/runs/6205301140 |
The two main changes are: