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

fix(CA2023: Adds validation against invalid braces in logger message templates) #7286

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

@Kritner Kritner requested a review from a team as a code owner April 10, 2024 13:36
Copy link

codecov bot commented Apr 10, 2024

Codecov Report

Attention: Patch coverage is 96.66667% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 96.48%. Comparing base (b07c100) to head (7a46e9f).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7286   +/-   ##
=======================================
  Coverage   96.48%   96.48%           
=======================================
  Files        1443     1443           
  Lines      345394   345483   +89     
  Branches    11364    11378   +14     
=======================================
+ Hits       333240   333330   +90     
+ Misses       9275     9274    -1     
  Partials     2879     2879           

@Kritner Kritner changed the title Adds validation against invalid bracket pairs fix(CA2017: Adds validation against invalid bracket pairs) Apr 10, 2024
@Kritner Kritner marked this pull request as draft April 10, 2024 21:58
@Kritner Kritner marked this pull request as ready for review April 11, 2024 02:57
@Kritner
Copy link
Author

Kritner commented Apr 11, 2024

https://dev.azure.com/dnceng-public/public/_build/results?buildId=639007&view=logs&j=f0f0520d-467b-5d1d-a95d-4e3a2581fe8a&t=c140c00f-6c0a-5356-d099-4e047a5525ed&l=221

image

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:

One or more auto-generated documentation files were either edited manually, or not updated. Please revert changes made to the following files (if manually edited) and run `msbuild /t:pack` at the root of the repo to automatically update them:
      D:\a\_work\1\s\src\NetAnalyzers\Microsoft.CodeAnalysis.NetAnalyzers.md
      D:\a\_work\1\s\src\NetAnalyzers\Microsoft.CodeAnalysis.NetAnalyzers.sarif

@Kritner
Copy link
Author

Kritner commented Apr 11, 2024

@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 release/8.0.2xx branch such that it will get release under the next build of those revision of packages? Or should this be targeting main?

(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)

@Kritner Kritner marked this pull request as draft April 11, 2024 18:30
@buyaa-n
Copy link
Member

buyaa-n commented Apr 11, 2024

I'm assuming i want to target my PR to the release/8.0.2xx branch such that it will get release under the next build of those revision of packages? Or should this be targeting main?

No this should target main, only servicing changes should target release branches

@Kritner Kritner changed the base branch from release/8.0.2xx to main April 11, 2024 23:06
ViktorHofer and others added 13 commits April 11, 2024 19:16
* 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
* Will likely need a follow up commit to "undo" the "new" tags around `CA2017`, as these should now match from main
…ated" versions from CA2017

* CA2017 was originally repurposed, which caused undesired impacts to already released versions of the analyzer. Instead these changes are introduced as a new CA2023 dotnet#7286 (comment)
@Kritner Kritner changed the title fix(CA2017: Adds validation against invalid bracket pairs) fix(CA2023: Adds validation against invalid bracket pairs) Apr 15, 2024
@Kritner Kritner marked this pull request as ready for review April 15, 2024 16:33
@Kritner
Copy link
Author

Kritner commented Apr 23, 2024

Hey just wanted to bump this to check if anything is needed from me, and/or what next steps are?

@tarekgh
Copy link
Member

tarekgh commented Apr 23, 2024

@stephentoub could you please advise if we can handle this in already existing rule or do you think we need to introduce a new analysis rule with new diagnostic?

@Kritner Kritner changed the title fix(CA2023: Adds validation against invalid bracket pairs) fix(CA2023: Adds validation against invalid braces in logger message templates) Apr 25, 2024
@tarekgh tarekgh marked this pull request as draft April 29, 2024 20:58
@tarekgh
Copy link
Member

tarekgh commented Apr 29, 2024

@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.

@Kritner
Copy link
Author

Kritner commented Apr 29, 2024

from @tarekgh:

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.

added dotnet/runtime#101698

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