-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Port inheritdoc tags for APIs that are already fully documented #8336
Conversation
…he only undocumented tags are either remarks or value.
@dotnet/area-extensions-caching @dotnet/area-extensions-configuration @dotnet/area-extensions-dependencyinjection @dotnet/area-extensions-filesystem @dotnet/area-extensions-hosting @dotnet/area-extensions-logging @dotnet/area-extensions-primitives @dotnet/area-system-buffers @dotnet/area-system-codedom @dotnet/ncl @HongGit @StephenMolloy @dotnet/area-system-runtime @dotnet/area-system-security @dotnet/area-system-text-json @dotnet/area-system-text-encoding @dotnet/area-system-xml @dotnet/area-system-globalization |
@carlossanlop does this change include all inherit docs or just a subset of it? I am asking because I recall there were some of them in DateOnly and component model which not showing up here. MS extensions and System changes LGTM. |
@tarekgh the main PR is this one: #8335, which contains the subset of APIs that are currently showing as undocumented in the report. The rest of them are the ones you're seeing in this PR. They are the APIs that have already been fully documented in the past, by copying the strings from the base type or the interface. |
This comment was marked as outdated.
This comment was marked as outdated.
I'm investigating the warnings. I see the tool is porting some crefs with characters escaped, and that should not happen. |
This comment was marked as outdated.
This comment was marked as outdated.
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 LGTM @carlossanlop
Once the open conversations are resolved, you can
Why do the APIs show up in the report if they are already documented? |
That's the description of the other PR, in which I am porting the inheritdoc tag of those APIs that were still partially or fully undocumented. So those are the ones showing up in the report. This PR contains APIs that are already fully documented, but the tool also found they had the inheritdoc tag in triple slash. These aren't showing up in the report, I'm just porting them for completeness. |
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.
The rest of the warnings are being checked by the Docs dev team, they seem to be a bug.
I wonder what mdoc would do in this case. Perhaps we should test. |
This comment was marked as outdated.
This comment was marked as outdated.
You were right to raise the concern. If we add inheritdoc to an API with partial or full documentation, inheritdoc is ignored. Example: The GUID operator "<=": Here is the preview: Here is the source file: Here are the partially documented docs: dotnet-api-docs/xml/System/Guid.xml Lines 1149 to 1156 in 83a7d69
Here is the interface from which it should inherit automatically: dotnet-api-docs/xml/System.Numerics/IComparisonOperators`3.xml Lines 82 to 83 in 83a7d69
So I guess the Docs porting tool should strip all the existing documentation (except remarks) if it is going to add an inheritdoc. What do you think, @gewarren? |
Docs Build status updates of commit 914ecb3: ✅ Validation status: passedThis comment lists only the first 25 files in the pull request. xml/Microsoft.Extensions.DependencyInjection/DefaultServiceProviderFactory.xml
xml/Microsoft.Extensions.FileProviders/NotFoundDirectoryContents.xml
xml/System.Net.Http.Headers/HeaderStringValues.xml
xml/System.Net.Http.Headers/HeaderStringValues+Enumerator.xml
xml/System.Net.Http.Headers/HttpHeadersNonValidated.xml
xml/System.Net.Http.Headers/HttpHeadersNonValidated+Enumerator.xml
xml/System.Text.Json/JsonElement+ArrayEnumerator.xml
xml/System.Text.Json/JsonElement+ObjectEnumerator.xml
xml/System/TimeOnly.xml
xml/System/TimeSpan.xml
This comment lists only the first 25 errors (including error/warning/suggestion) in the pull request. Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report. For any questions, please:
|
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.
System.Net.* changes LGTM
But isn't there a bug with inheritdoc on operators specifically? It actually does seem to be working properly (here the value is inherited): https://review.docs.microsoft.com/en-us/dotnet/api/system.net.http.headers.headerstringvalues.enumerator.current?view=net-7.0&branch=pr-en-us-8336. So I guess this PR is okay. |
Ah, you're right. It's basically the same problem as the bug we opened: the parent API to inherit from is not found, so it's still considered undocumented, wrongly.
Thank you for checking. I'll merge. |
This is a continuation of #8335 . Full explanation can be found there.
This PR includes the tags that were found in APIs that were already documented.