-
Notifications
You must be signed in to change notification settings - Fork 6k
Repo cleanup - May #29498
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
Repo cleanup - May #29498
Conversation
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
@BillWagner Is this a good spot for the warning waves doc? https://review.docs.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-messages/warning-waves?branch=pr-en-us-29498 Also, the AnalysisLevel property seems to be shared between C# compiler messages and Roslyn code analysis. I'll log an issue to make note of this in the docs. |
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.
Thanks @gewarren
This looks great.
I left comments around the C# compiler warning TOC. I can open a separate PR for those if you'd prefer.
@@ -1561,6 +1561,8 @@ items: | |||
href: cs8410.md | |||
- name: CS8411 | |||
href: cs8411.md | |||
- name: Warning waves |
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.
Great catch @gewarren I can't believe I missed adding this.
This new article should replace the node "Level 5 warning messages", starting at line 1882. I'll add a second comment there.
I can open a separate PR for this if you want.
- name: Level 5 warning messages | ||
items: | ||
- name: CS8892 | ||
href: cs8892.md | ||
href: warning-waves.md#cs8892---method-will-not-be-used-as-an-entry-point-because-a-synchronous-entry-point-method-was-found |
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.
This section should be removed and replaced with the warning waves
Line 1384 and 1385 should go here. In addition, the displayName
metadata should list all the warning IDs in that file.
Should I open a separate PR for this?
items: | ||
- name: CS8892 | ||
href: cs8892.md | ||
- name: Warning waves |
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.
@BillWagner Does this look correct?
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.
That's perfect! Let's
Contributes to #29115