Skip to content

[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

Merged
merged 7 commits into from
Nov 4, 2022

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Oct 13, 2022

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.:
image

Regression?

  • Yes
  • No

Risk

  • High
  • Medium
  • Low

The change only updates the API documentation that will reflect in the Learn portal.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

@brunolins16 brunolins16 added the Servicing-consider Shiproom approval is required for the issue label Oct 14, 2022
@ghost
Copy link

ghost commented Oct 14, 2022

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.

@adityamandaleeka
Copy link
Member

@danmoseley I assume we can skip shiproom for this since it's just changing doc comments.

@danmoseley
Copy link
Member

At this stage the concern might be the risk of any change. How close are we to final build?

@brunolins16
Copy link
Member

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.

@dougbu
Copy link
Contributor

dougbu commented Oct 20, 2022

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.

@rbhanda rbhanda added this to the 7.0.1 milestone Oct 20, 2022
@rbhanda rbhanda added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Oct 20, 2022
@danmoseley
Copy link
Member

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?

@dougbu
Copy link
Contributor

dougbu commented Oct 20, 2022

Not sure when the Learn Portal gets updated. However, wouldn't this PR improve the IntelliSense experience❔

@brunolins16
Copy link
Member

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.

@danmoseley
Copy link
Member

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)

@dougbu
Copy link
Contributor

dougbu commented Oct 20, 2022

Can you find out when the portal is updated and whether a change post GA would even make any difference?

Who has this action item❔ xakep139 doesn't seem to work in Microsoft.

@adityamandaleeka
Copy link
Member

@Rick-Anderson likely knows the answer to that^

@Rick-Anderson
Copy link
Contributor

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)

The /dotnet/api doc's aren't changed post GA unless we order a refresh, which is easy to do. AFAIK, that would only impact the 7.0 API docs. cc @v-alje @gewarren

/// <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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// <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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// <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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// 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:

@danmoseley
Copy link
Member

unless we order a refresh, which is easy to do.

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.

@v-alje
Copy link

v-alje commented Oct 20, 2022

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.

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.

@danmoseley
Copy link
Member

OK, sounds like there's indeed benefit in including these two PRs in 7.0.1. No objections from me.

@brunolins16
Copy link
Member

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

@brunolins16
Copy link
Member

@dotnet/aspnet-build since branch is open, can someone help me with this.

@wtgodbe
Copy link
Member

wtgodbe commented Nov 2, 2022

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

@dougbu dougbu merged commit 761eec9 into release/7.0 Nov 4, 2022
@dougbu dougbu deleted the backport/pr-44511-to-release/7.0 branch November 4, 2022 03:10
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants