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

Extract long remarks #9535

Merged
merged 15 commits into from
Jan 12, 2024
Merged

Extract long remarks #9535

merged 15 commits into from
Jan 12, 2024

Conversation

gewarren
Copy link
Contributor

@gewarren gewarren commented Jan 6, 2024

Contributes to dotnet/docs#37859

@ghost ghost assigned gewarren Jan 6, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-Meta Concerns something that extends across runtime area boundaries, for example, IDisposable. label Jan 6, 2024
@dotnet dotnet deleted a comment from learn-build-service-prod bot Jan 6, 2024
@dotnet dotnet deleted a comment from learn-build-service-prod bot Jan 6, 2024

This comment was marked as outdated.

This comment was marked as outdated.

@gewarren gewarren closed this Jan 9, 2024
@gewarren gewarren reopened this Jan 9, 2024

This comment was marked as outdated.

@gewarren gewarren closed this Jan 10, 2024
@gewarren gewarren reopened this Jan 10, 2024

This comment was marked as outdated.

This comment was marked as outdated.

@gewarren gewarren closed this Jan 10, 2024
@gewarren gewarren reopened this Jan 10, 2024

This comment was marked as outdated.

This comment was marked as outdated.

@gewarren gewarren marked this pull request as ready for review January 11, 2024 04:26
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

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

System.Runtime.InteropServices LGTM

@ManickaP
Copy link
Member

I know this will not change anything and I don't expect it to, but I wanted to express my opinion on this. I think this just inserts another obstacle between the user and the information they need. IMO API docs are the gateway to .NET docs. If the information is not in the API docs, it doesn't exist for many users. And lately, we've been slowly removing valuable information from API docs and just leaving the useless "Gets or sets the name of the object" behind. And it makes me sad, because these docs used to be one of the best I ever used.

@antonfirsov
Copy link
Member

antonfirsov commented Jan 11, 2024

Regarding @ManickaP's comment I think the goal here is to find a balance between usability on the customer side and maintainability on our side. I assume there are good reasons for keeping /// docs fully synchronized, but embedding long API remarks into dotnet/runtime code sounds like a nightmare.

IMO the ideal solution would be to maintain long remarks separately (still in this repo), but make the tooling copy the remarks into the resulting API doc html pages.

@gewarren
Copy link
Contributor Author

gewarren commented Jan 11, 2024

@ManickaP Now is the time to get this right, so your feedback matters. It sounds like most everyone including @carlossanlop @stephentoub thinks the best solution is for all the Remarks (or perhaps anything longer than a couple sentences) to be published inline via Include files that live in this (dotnet-api-docs) repo. (For 1-2 sentence remarks, those can probably live directly in the ///?)

The purpose of this PR was to extract any really, really long remarks into the conceptual docs, to ease page navigation and to utilize all the grammar/link checks we get in the conceptual docs repo. However, if the team doesn't like even this small subset of remarks being moved to conceptual docs, they can certainly be brought in via Include files as well.

The downsides that I see for using Include files are that 1) they'd require PRs in this (dotnet-api-docs) repo to edit, and once /// is source of truth, edits would otherwise be disabled in this repo and 2) long remarks are difficult to navigate in the API docs presentation.

we've been slowly removing valuable information from API docs

Regarding this comment, can you elaborate on how information has been removed (other than this PR)? Or is it just that information hasn't been added for the newer APIs?

This comment was marked as outdated.

Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

Left some comments for you to consider. Feel free to address them in a separate PR if you prefer. Looks good enough for merging as is.

Copy link

Learn Build status updates of commit 41600dd:

⚠️ Validation status: warnings

File Status Preview URL Details
xml/System.Globalization/CompareInfo.xml ⚠️Warning View Details
xml/System.Globalization/CultureAndRegionInfoBuilder.xml ⚠️Warning View Details
xml/System.Globalization/CultureInfo.xml ⚠️Warning View Details
xml/System.Globalization/DateTimeFormatInfo.xml ⚠️Warning View Details
xml/System.Globalization/NumberFormatInfo.xml ⚠️Warning View Details
xml/System.Globalization/RegionInfo.xml ⚠️Warning View Details
xml/System.Globalization/SortVersion.xml ⚠️Warning View Details
xml/System/Uri.xml 💡Suggestion View Details
add/media/aboutgdip05-art10.gif ✅Succeeded n/a (file deleted or renamed)
add/media/aboutgdip05-art12.gif ✅Succeeded n/a (file deleted or renamed)
xml/System.Collections.Generic/HashSet`1.xml ✅Succeeded View
xml/System.Collections.Generic/List`1.xml ✅Succeeded View
xml/System.Collections.ObjectModel/ObservableCollection`1.xml ✅Succeeded View
xml/System.Data.Linq.Mapping/TableAttribute.xml ✅Succeeded View
xml/System.Data.Linq/DataLoadOptions.xml ✅Succeeded View
xml/System.Data.Objects/MergeOption.xml ✅Succeeded View
xml/System.Data/CommandBehavior.xml ✅Succeeded View
xml/System.Data/DataSet.xml ✅Succeeded View
xml/System.Data/DataTable.xml ✅Succeeded View
xml/System.Diagnostics.Tracing/EventSource.xml ✅Succeeded View
xml/System.Diagnostics.Tracing/EventWrittenEventArgs.xml ✅Succeeded View
xml/System.Diagnostics/PerformanceCounterType.xml ✅Succeeded View
xml/System.Drawing.Drawing2D/Matrix.xml ✅Succeeded View
xml/System.Dynamic/ExpandoObject.xml ✅Succeeded View
xml/System.Globalization/CompareOptions.xml ✅Succeeded View

This comment lists only the first 25 files in the pull request.

xml/System.Globalization/CompareInfo.xml

  • Line 0, Column 0: [Warning: disallowed-html-tag - See documentation] HTML tag 'format' isn't allowed. Replace it with approved Markdown or escape the brackets if the content is a placeholder.

xml/System.Globalization/CultureAndRegionInfoBuilder.xml

  • Line 0, Column 0: [Warning: disallowed-html-tag - See documentation] HTML tag 'format' isn't allowed. Replace it with approved Markdown or escape the brackets if the content is a placeholder.

xml/System.Globalization/CultureInfo.xml

  • Line 0, Column 0: [Warning: disallowed-html-tag - See documentation] HTML tag 'format' isn't allowed. Replace it with approved Markdown or escape the brackets if the content is a placeholder.

xml/System.Globalization/DateTimeFormatInfo.xml

  • Line 0, Column 0: [Warning: disallowed-html-tag - See documentation] HTML tag 'format' isn't allowed. Replace it with approved Markdown or escape the brackets if the content is a placeholder.

xml/System.Globalization/NumberFormatInfo.xml

  • Line 0, Column 0: [Warning: disallowed-html-tag - See documentation] HTML tag 'format' isn't allowed. Replace it with approved Markdown or escape the brackets if the content is a placeholder.

xml/System.Globalization/RegionInfo.xml

  • Line 0, Column 0: [Warning: disallowed-html-tag - See documentation] HTML tag 'format' isn't allowed. Replace it with approved Markdown or escape the brackets if the content is a placeholder.

xml/System.Globalization/SortVersion.xml

  • Line 0, Column 0: [Warning: disallowed-html-tag - See documentation] HTML tag 'format' isn't allowed. Replace it with approved Markdown or escape the brackets if the content is a placeholder.

xml/System/Uri.xml

  • Line 0, Column 0: [Suggestion: ECMA2Yaml_Inheritdoc_NoFoundParent] Found no member can be inherited by key:System#IFormattable#ToString(System.String,System.IFormatProvider) for uid: System.Uri.System#IFormattable#ToString(System.String,System.IFormatProvider).
  • Line 0, Column 0: [Suggestion: ECMA2Yaml_Inheritdoc_NoFoundParent] Found no member can be inherited by key:System#ISpanFormattable#TryFormat(System.Span{System.Char},System.Int32@,System.ReadOnlySpan{System.Char},System.IFormatProvider) for uid: System.Uri.System#ISpanFormattable#TryFormat(System.Span{System.Char},System.Int32@,System.ReadOnlySpan{System.Char},System.IFormatProvider).

For more details, please refer to the build report.

Note: Your PR may contain errors or warnings or suggestions unrelated to the files you changed. This happens when external dependencies like GitHub alias, Microsoft alias, cross repo links are updated. Please use these instructions to resolve them.

For any questions, please:

@gewarren gewarren merged commit 4fc03a2 into dotnet:main Jan 12, 2024
3 checks passed
@gewarren gewarren deleted the remarks-2 branch January 12, 2024 00:15
@ManickaP
Copy link
Member

Regarding this comment, can you elaborate on how information has been removed (other than this PR)? Or is it just that information hasn't been added for the newer APIs?

It's both. For example, I rarely see us adding example usages with new APIs. And with removing the info, for example here and here where we removed code samples because they were out of date, instead of updating them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Meta Concerns something that extends across runtime area boundaries, for example, IDisposable.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants