-
Notifications
You must be signed in to change notification settings - Fork 10.4k
[release/7.0] Fixing XML documentation #44523
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
Conversation
Hi @github-actions[bot]. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge. To learn more about how to prepare a servicing PR click here. |
@danmoseley I assume we can skip shiproom for this since it's just changing doc comments. |
At this stage the concern might be the risk of any change. How close are we to final build? |
I am not sure how close we are but @dougbu might have more information. |
I believe we've got a final GA build. Given this isn't servicing approved and we're in the middle of the repo stack, I suggest waiting for 7.0.1 unless @mmitche is enthusiastic. |
discussed in tactics, and at this point we want to take for 7.0.1. however, I want to check -- is there a documentation update in servicing? ie., would this even have an effect in docs if we don't take in GA? |
Not sure when the Learn Portal gets updated. However, wouldn't this PR improve the IntelliSense experience❔ |
No. The IntelliSense experience is not broken right now. |
Can you find out when the portal is updated and whether a change post GA would even make any difference? If not, we wouldn't take this (and the linked one - maybe we should have combined the PR's) |
Who has this action item❔ |
@Rick-Anderson likely knows the answer to that^ |
The |
/// <item>it has annotations (e.g. BindRequiredAttribute) that indicate it's required.</item> | ||
/// <item><description>it's bound from the request body (<see cref="BindingSource.Body"/>).</description></item> | ||
/// <item><description>it's a required route value.</description></item> | ||
/// <item><description>it has annotations (e.g. BindRequiredAttribute) that indicate it's required.</description></item> |
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.
/// <item><description>it has annotations (e.g. BindRequiredAttribute) that indicate it's required.</description></item> | |
/// <item><description>it has annotations (for example, BindRequiredAttribute) that indicate it's required.</description></item> |
/// <list type="bullet">One of them reports that the response was written successfully, or.</list> | ||
/// <list type="bullet">All <see cref="IProblemDetailsWriter"/> were executed and none of them was able to write the response successfully.</list> | ||
/// <list type="bullet"> | ||
/// <item><description>One of them reports that the response was written successfully, or.</description></item> |
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.
/// <item><description>One of them reports that the response was written successfully, or.</description></item> | |
/// <item><description>One of them reports that the response was written successfully.</description></item> |
@@ -16,8 +16,10 @@ public interface IProblemDetailsService | |||
/// <param name="context">The <see cref="ProblemDetailsContext"/> associated with the current request/response.</param> | |||
/// <remarks>The <see cref="IProblemDetailsWriter"/> registered services | |||
/// are processed in sequence and the processing is completed when: |
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.
/// are processed in sequence and the processing is completed when: | |
/// are processed in sequence and the processing is completed when either of the follow occurs: |
Is that a well exercised, well understood process? I'm just wondering whether we'd be doing something "unusual" by requesting this on a servicing release, and that would mean it would potentially create some problem. Incidentally, when do you normally start to throw up docs for the next release? Eg., as soon as .NET 8 preview 1? I ask because presumably when those are up, they will include the "corrected" docs. |
Updating for a service release is the same process as for a preview or full release. The configurations are currently set to pull in the changes for the latest preview version. Once the full release publishes, we will need to remove preview from the configuration then it will be the latest packages until previews for 8 publish. When the NuGet packages for 8 publish, we will need to lock in 7 to the latest service release and manually update the version numbers in the configuration file as needed. Yes, once we pick up the packages for 8, the text in the XML content of those NuGet packages will replace the older documentation. |
OK, sounds like there's indeed benefit in including these two PRs in 7.0.1. No objections from me. |
OK, I will work to have both PRs merged when 7.0.1. Thanks |
@dotnet/aspnet-build since branch is open, can someone help me with this. |
7.0 in this repo isn't open yet, there are some issues with the branding PR. We'll merge this once that's resolved |
Backport of #44511 to release/7.0
/cc @brunolins16
Fixing XML documentation
Description
Updates some api comments to correctly be processed and show in the Learn Portal
Fixes #44428
Customer Impact
Without the change the documentation is really bad and only include empty bullets.
Eg.:

Regression?
Risk
The change only updates the API documentation that will reflect in the Learn portal.
Verification
Packaging changes reviewed?