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

Make System.Formats.Asn1 triple slash documentation the source of truth #108982

Merged
merged 6 commits into from
Oct 17, 2024

Conversation

vcsjones
Copy link
Member

The pull request makes the triple slash documentation for System.Formats.Asn1 the source of truth for documentation.

  1. Use the PortToTripleSlash tool to update the C# documentation with the latest documentation from dotnet-api-docs.

    This made substantial changes that were largely formatting, which I reverted. I kept the changes to two things: actual content changes (text changes, missing documentation, etc) and re-ordering XML elements as appropriate (param elements not matching parameter order, putting remarks at the end)

  2. Remove <UseCompilerGeneratedDocXmlFile>false</UseCompilerGeneratedDocXmlFile>. The default value for this is true. I can't remember where, but I recall that our preference is to just remove it entirely, not explicitly set it to true.

Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-formats-asn1, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

@vcsjones
Copy link
Member Author

cc @carlossanlop in case I missed anything.

Copy link
Contributor

@gewarren gewarren left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

@@ -41,15 +41,15 @@ public static partial class AsnDecoder
/// <paramref name="ruleSet"/> is not defined.
/// </exception>
/// <exception cref="AsnContentException">
/// the next value does not have the correct tag.
/// The next value does not have the correct tag.
Copy link
Contributor

Choose a reason for hiding this comment

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

These might need <para> tags so they remain on separate lines in the docs, right @carlossanlop?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see now that that would mean a lot of changes throughout this PR. I'm also fine with not adding them.

Copy link
Member Author

Choose a reason for hiding this comment

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

The porttotripleslash tool did not put <para> there, so I assumed it was not required.

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.

LGTM (modulo @gewarren's suggestions). Can you please backport it to release/9.0 when you're done?

vcsjones and others added 2 commits October 17, 2024 13:40
Co-authored-by: Genevieve Warren <[email protected]>
Co-authored-by: Jeremy Barton <[email protected]>
Co-authored-by: Jeremy Barton <[email protected]>
Co-authored-by: Genevieve Warren <[email protected]>
@gewarren
Copy link
Contributor

@carlossanlop Is there a documented set of steps needed to set this repo as the source of truth? If so, can you add a step to disallow edits in the dotnet-api-docs repo for the affected namespace? Like this.

@ViktorHofer
Copy link
Member

The default value for this is true. I can't remember where, but I recall that our preference is to just remove it entirely, not explicitly set it to true.

Correct 👍 and big thanks for doing this.

@vcsjones vcsjones merged commit f2f3133 into dotnet:main Oct 17, 2024
79 of 83 checks passed
@vcsjones vcsjones deleted the asn1-docs branch October 17, 2024 19:52
@vcsjones
Copy link
Member Author

/backport to release/9.0

Copy link
Contributor

Started backporting to release/9.0: https://github.com/dotnet/runtime/actions/runs/11392049693

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants