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

Enabled logging analyzer on new repo #5244

Merged
merged 22 commits into from
Jul 28, 2021

Conversation

maryamariyan
Copy link
Member

@maryamariyan maryamariyan commented Jul 14, 2021

@maryamariyan maryamariyan requested a review from a team as a code owner July 14, 2021 23:35
@maryamariyan
Copy link
Member Author

maryamariyan commented Jul 15, 2021

Got caught up with conflicts from diagnostics on other PRs getting merged and me having to change my diagnostic numbers on this PR.

Currently checking to see if the CI build is successful before pushing up a WIP commit trying to address using operation analysis instead of syntax analysis.

Once that is done, I will address the remaining feedback.

@codecov
Copy link

codecov bot commented Jul 15, 2021

Codecov Report

Merging #5244 (cf7cc42) into release/6.0.1xx (d36599d) will decrease coverage by 0.02%.
The diff coverage is 81.87%.

@@                 Coverage Diff                 @@
##           release/6.0.1xx    #5244      +/-   ##
===================================================
- Coverage            95.62%   95.60%   -0.03%     
===================================================
  Files                 1233     1236       +3     
  Lines               283449   283895     +446     
  Branches             16968    17032      +64     
===================================================
+ Hits                271052   271418     +366     
- Misses               10108    10179      +71     
- Partials              2289     2298       +9     

@maryamariyan
Copy link
Member Author

the MemberData test cases for CA2255IsProducedForFormatArgumentCountMismatch fail on the latest update. Looking into it.

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.

Since there is no language specific logic now. I believe this can be moved up from CSharp to Core without any extra work other than unit tests.

@maryamariyan
Copy link
Member Author

The test failures still need looking into. @sharwell and I plan on looking at this again today.

@Youssef1313
Copy link
Member

@maryamariyan I'm not seeing test failures in CI. The failure in CI is due to auto-generated files being outdated. Can you run msbuild /t:pack /v:m in the repository root? This should update the following files:

  • src\NetAnalyzers\Microsoft.CodeAnalysis.NetAnalyzers.md
  • src\NetAnalyzers\Microsoft.CodeAnalysis.NetAnalyzers.sarif

@maryamariyan
Copy link
Member Author

The PR is ready for review :)

@maryamariyan
Copy link
Member Author

@maryamariyan if this is going to be enabled by default in the .NET 6 SDK I suggest we add a performance test for it here: https://github.com/dotnet/roslyn-analyzers/tree/main/src/PerformanceTests/Tests/Enabled this can happen in a follow up PR in the RC1/RC2 timeframe

OK, will address in a follow up PR

@jmarolf jmarolf enabled auto-merge July 28, 2021 17:19
@buyaa-n
Copy link
Member

buyaa-n commented Jul 28, 2021

for future reference here is a sample set exercising all 5 diagnostics assuming all are enabled:

The message structure looks good to me, but the Named holes sounds weird, not sure that is the term used for C# cc @terrajobst

auto-merge was automatically disabled July 28, 2021 17:33

Head branch was pushed to by a user without write access

@maryamariyan
Copy link
Member Author

The message structure looks good to me, but the Named holes sounds weird, not sure that is the term used for C# cc @terrajobst

I renamed to "named placeholder" from "named hole", since that works as well

@jmarolf jmarolf enabled auto-merge July 28, 2021 17:50
@jmarolf jmarolf merged commit 7445fb3 into dotnet:release/6.0.1xx Jul 28, 2021
@maryamariyan maryamariyan deleted the logging-az branch July 28, 2021 18:40
@terrajobst
Copy link
Member

I renamed to "named placeholder" from "named hole", since that works as well

💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants