-
Notifications
You must be signed in to change notification settings - Fork 464
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
fix(CA2023: Adds validation against invalid braces in logger message templates) #7286
Conversation
Kritner
commented
Apr 10, 2024
- Fixes Malformed message template string should throw warning or compiler error as a preference over runtime exceptions #7285
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7286 +/- ##
==========================================
- Coverage 96.49% 96.49% -0.01%
==========================================
Files 1443 1443
Lines 345885 345946 +61
Branches 11374 11383 +9
==========================================
+ Hits 333765 333823 +58
+ Misses 9241 9240 -1
- Partials 2879 2883 +4 |
getting this build error, both prior to and after making updates to the files mentioned. I'm not sure if this command is supposed to be run locally, or as a part of the build automatically; but i was getting errors attempting to run it locally. going to revert changes to these two files mentioned and just double check the same error occurs:
|
@buyaa-n i think this might be good to go now - though i'm a bit unsure of the git flow being used. I'm assuming i want to target my PR to the (I only tagged you because of your activity on #7285, lmk if i should tag someone else or just let this thing sit til it gets through some queue) |
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/LoggerMessageDefineAnalyzer.cs
Outdated
Show resolved
Hide resolved
No this should target main, only servicing changes should target release branches |
This reverts commit e640355.
This reverts commit 4c41c4e.
* Needed to have a title and description that match *both* potential reasons for this warning * The individual messages for the differing reasons is still separate, but the MD/sarif description seemed to be "last wins" when it comes to a title/description * The `msbuild /t:pack` command kept failing for me, so upped the global.json to target a non preview .net8 SDK, but am not checking that change in
78c69e7
to
211c166
Compare
@Kritner thanks for helping with this issue. If we'll continue having a new diagnostic we'll need to get approval for it before we proceed. Could you please log a new issue in the runtime repo to track reviewing it? here is some example issue dotnet/runtime#78406 which you can mimic. meanwhile I converted this PR to draft till we finalize the process. |
from @tarekgh:
added dotnet/runtime#101698 |
...etAnalyzers/Core/Microsoft.CodeQuality.Analyzers/MicrosoftCodeQualityAnalyzersResources.resx
Outdated
Show resolved
Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/LoggerMessageDefineAnalyzer.cs
Outdated
Show resolved
Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/LoggerMessageDefineAnalyzer.cs
Show resolved
Hide resolved
Think i handled all the comments from the PR review, wasn't sure if i should be the one "resolving conversations" or if you'd be doing that @tarekgh. Thanks for the help! |
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 @Kritner!
@buyaa-n could you please have a quick look and merge if it looks good to you too? |
Is this analyzer needed for v9? I would wait until the main open for v10. |
@buyaa-n could you please track it to merge when the main open for v10 merges? |
I plan to go through all PRs and review/merge when main opens for v10 |
Added a few more covering tests based on the new implementation |
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.
Looks good, thank you @Kritner!