Skip to content

Update DocumentationMode docs#52675

Merged
333fred merged 2 commits intodotnet:mainfrom
Youssef1313:patch-21
Apr 16, 2021
Merged

Update DocumentationMode docs#52675
333fred merged 2 commits intodotnet:mainfrom
Youssef1313:patch-21

Conversation

@Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 requested a review from a team as a code owner April 16, 2021 10:13
@ghost ghost added the Area-Compilers label Apr 16, 2021

/// <summary>
/// Gets a value indicating whether the documentation comments are parsed.
/// Gets a value indicating whether the documentation comments are parsed and can produce diagnostics, parsed only, or treated as regular comments.
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider simply saying: "Gets a value indicating whether the documentation comments are parsed and analyzed."

/// </summary>
/// <value><c>true</c> if documentation comments are parsed, <c>false</c> otherwise.</value>
/// <value>
/// <para><see cref="DocumentationMode.Diagnose"/> if documentation comments are parsed and can produce diagnostics.</para>
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider saying: "if documentation comments are parsed and analyzed"

/// <value><c>true</c> if documentation comments are parsed, <c>false</c> otherwise.</value>
/// <value>
/// <para><see cref="DocumentationMode.Diagnose"/> if documentation comments are parsed and can produce diagnostics.</para>
/// <para><see cref="DocumentationMode.Parse"/> if documentation comments are parsed without producing diagnostics.</para>
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider saying: "if documentation comments are parsed, but not analyzed." I am assuming parsing itself can cause some diagnostics to be reported. So, "without producing diagnostics" is probably not accurate.

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 doc for DocumentationMode.Parse says: "Parses documentation comments as structured trivia, but do not report any diagnostics.". I'll try to check if parsing itself can produce diagnostics or not, and update both comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

If enum values are documented, then why duplicate the information here at all? This is going to create an unnecessary maintenance burden, in my opinion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds reasonable. I'm going to delete the "value" tag.

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 1)

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 2)

@AlekseyTs
Copy link
Contributor

@dotnet/roslyn-compiler For a second review.

@333fred 333fred merged commit 883ec07 into dotnet:main Apr 16, 2021
@ghost ghost added this to the Next milestone Apr 16, 2021
@333fred
Copy link
Member

333fred commented Apr 16, 2021

Thanks @Youssef1313!

@Youssef1313 Youssef1313 deleted the patch-21 branch April 17, 2021 02:16
@dibarbet dibarbet modified the milestones: Next, 16.10.P3 Apr 26, 2021
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.

It's an enum, not a bool

4 participants