-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Extensions: xml docs #78365
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
Extensions: xml docs #78365
Conversation
166abd7
to
074b68e
Compare
{ | ||
if (symbol is SourceExtensionImplementationMethodSymbol implementation) | ||
{ | ||
var symbolForDocComment = implementation.UnderlyingMethod is SourcePropertyAccessorSymbol accessorSymbol |
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.
? accessorSymbol.AssociatedSymbol | ||
: implementation.UnderlyingMethod; | ||
|
||
if (symbolForDocComment.GetDocumentationCommentXml(preferredCulture: null, _processIncludes, _cancellationToken).IsEmpty()) |
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.
? accessorSymbol.AssociatedSymbol | ||
: implementation.UnderlyingMethod; | ||
|
||
if (symbolForDocComment.GetDocumentationCommentXml(preferredCulture: null, _processIncludes, _cancellationToken).IsEmpty()) |
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.
return; | ||
} | ||
|
||
DefaultVisit(symbol); |
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.
@@ -131,6 +135,11 @@ internal override int TryGetOverloadResolutionPriority() | |||
return UnderlyingMethod.TryGetOverloadResolutionPriority(); | |||
} | |||
|
|||
public override string GetDocumentationCommentXml(CultureInfo? preferredCulture = null, bool expandIncludes = false, CancellationToken cancellationToken = default) | |||
{ | |||
return SourceDocumentationCommentUtils.GetAndCacheDocumentationComment(this, expandIncludes, ref lazyDocComment); |
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.
@@ -131,6 +135,11 @@ internal override int TryGetOverloadResolutionPriority() | |||
return UnderlyingMethod.TryGetOverloadResolutionPriority(); | |||
} | |||
|
|||
public override string GetDocumentationCommentXml(CultureInfo? preferredCulture = null, bool expandIncludes = false, CancellationToken cancellationToken = default) |
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.
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.
There's multiple direct tests ( XmlDoc_01
through XmlDoc_03
). Do you have specific API tests in mind?
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.
Do you have specific API tests in mind?
Yes, the effect of expandIncludes
and effect of caching, including on consecutive calls with different expandIncludes
.
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.
Added an assertion
{ | ||
if (memberSyntax is BaseMethodDeclarationSyntax { ParameterList: { ParameterCount: > 0 } } baseMethodDeclSyntax) | ||
{ | ||
Binder outerBinder = VisitCore(memberSyntax.Parent); | ||
MethodSymbol method = GetMethodSymbol(baseMethodDeclSyntax, outerBinder); | ||
|
||
if (isParamRef && method.TryGetInstanceExtensionParameter(out var _)) |
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.
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.
From an implementation perspective, this condition ensures that <param name="extensionParameter">
may only be placed on the extension block. This is shown in XmlDoc_Param_03
.
From a language perspective, this rule ensures that you don't end up with two <param>
tags for the extension parameter (one on the extension block and one on a member). I think allowing such duplicates would make consumption of the resulting xml more difficult than it needs to be.
Done with review pass (commit 1) |
string result = SourceDocumentationCommentUtils.GetAndCacheDocumentationComment(this, expandIncludes: false, ref lazyDocComment); | ||
|
||
#if DEBUG | ||
string withIncludes = DocumentationCommentCompiler.GetDocumentationCommentXml(this, processIncludes: true, default); |
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.
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.
LGTM (commit 4)
Recent decisions in LDM and dotnet/roslyn#78365 change how XML output is generated and should be consumed.
* Update docs output Recent decisions in LDM and dotnet/roslyn#78365 change how XML output is generated and should be consumed. * Proofread and clarify * Apply suggestions from code review Co-authored-by: Julien Couvreur <[email protected]> * respond to feedback. * Apply suggestions from code review Co-authored-by: Genevieve Warren <[email protected]> * Respond to feedback. --------- Co-authored-by: Julien Couvreur <[email protected]> Co-authored-by: Genevieve Warren <[email protected]>
Design (confirmed with Cyrus and Bill):
Note: I had a commit to switch
GetDocumentationCommentXml
fromvirtual
toabstract
(push the implementations into the leaves) but decided to separate such refactoring out.Relates to test plan #76130