-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add support for inheritdoc tags #3016
Conversation
…cursively using the `inheritdoc` as a reference. Point all XPath queries to this class. Simplify some of the (if) nesting in the XML Comment Filters. Add unit tests for the new InheritDoc updates. some existing tests were converted from `Fact` to `Theory` to accomplish this.
src/Swashbuckle.AspNetCore.Annotations/Swashbuckle.AspNetCore.Annotations.csproj
Outdated
Show resolved
Hide resolved
src/Swashbuckle.AspNetCore.SwaggerGen/XmlComments/XmlCommentsParameterFilter.cs
Outdated
Show resolved
Hide resolved
src/Swashbuckle.AspNetCore.SwaggerGen/XmlComments/XmlCommentsRecursion.cs
Outdated
Show resolved
Hide resolved
private const string SummaryTag = "summary"; | ||
private const string RemarksTag = "remarks"; | ||
private const string ResponseTag = "response"; |
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.
Define all these constants somewhere shared for re-use?
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.
I can definitely do this. Do you have a place in particular you prefer? Would you like me to also find and replace all instances of these static string values in the solution?
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.
Something like a XmlCommentTagNames
class in this namespace?
I don't think we need to go churning the whole codebase to use it though, particularly outside of this assembly.
src/Swashbuckle.AspNetCore.SwaggerGen/XmlComments/XmlCommentsOperationFilter.cs
Outdated
Show resolved
Hide resolved
src/Swashbuckle.AspNetCore.SwaggerGen/XmlComments/XmlCommentsRecursion.cs
Outdated
Show resolved
Hide resolved
src/Swashbuckle.AspNetCore.SwaggerGen/XmlComments/XmlCommentsRecursion.cs
Outdated
Show resolved
Hide resolved
src/Swashbuckle.AspNetCore.SwaggerGen/XmlComments/XmlCommentsRecursion.cs
Outdated
Show resolved
Hide resolved
// Try to find the specified tag node within the current member node. | ||
var node = memberNode?.SelectSingleNode(tag); | ||
if (node != null) | ||
return memberNode; |
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.
Took me a second to understand what this was doing. Given memberNode
might be null
I think this would be more clearly written as:
// Try to find the specified tag node within the current member node. | |
var node = memberNode?.SelectSingleNode(tag); | |
if (node != null) | |
return memberNode; | |
if (memberNode is null) | |
{ | |
return null; | |
} | |
// Try to find the specified tag node within the current member node. | |
var node = memberNode.SelectSingleNode(tag); | |
if (node != null) | |
{ | |
return memberNode; | |
} |
src/Swashbuckle.AspNetCore.SwaggerGen/XmlComments/XmlCommentsRecursion.cs
Outdated
Show resolved
Hide resolved
…les. This was out of scope.
…arameterFilter.cs Moved to a single line as requested Co-authored-by: Martin Costello <martin@martincostello.com>
…perationFilter.cs Add braces to `if` statement Co-authored-by: Martin Costello <martin@martincostello.com>
…equestBodyFilter.cs Clean up formatting. Co-authored-by: Martin Costello <martin@martincostello.com>
…equestBodyFilter.cs Co-authored-by: Martin Costello <martin@martincostello.com>
…chemaFilter.cs Add braces to if statement Co-authored-by: Martin Costello <martin@martincostello.com>
… returns are wrapped in braces. Update string.Format to use `CultureInfo.InvariantCulture`
@@ -17,27 +19,18 @@ public void Apply(OpenApiSchema schema, SchemaFilterContext context) | |||
{ | |||
ApplyTypeTags(schema, context.Type); | |||
|
|||
if (context.MemberInfo != null) | |||
if (context.MemberInfo != null) |
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.
if (context.MemberInfo != null) | |
if (context.MemberInfo != null) |
/// <inheritdoc cref = "FakeControllerWithXmlComments" /> | ||
public class FakeControllerWithInheritDoc : FakeControllerWithXmlComments | ||
{ | ||
/// <inheritdoc cref = "FakeControllerWithXmlComments.ActionWithSummaryAndRemarksTags"/> | ||
public new void ActionWithSummaryAndRemarksTags() | ||
{ | ||
throw new NotImplementedException(); | ||
} | ||
|
||
/// <inheritdoc cref = "FakeControllerWithXmlComments.ActionWithParamTags"/> | ||
public new void ActionWithParamTags(string param1, string param2) | ||
{ | ||
throw new NotImplementedException(); | ||
} | ||
|
||
/// <inheritdoc cref = "FakeControllerWithXmlComments.ActionWithResponseTags"/> |
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.
Only because I've never seen tags formatted this way before, so I think it just looks odd and out-of-place (I haven't seen if there's any already in this codebase 😄)
/// <inheritdoc cref = "FakeControllerWithXmlComments" /> | |
public class FakeControllerWithInheritDoc : FakeControllerWithXmlComments | |
{ | |
/// <inheritdoc cref = "FakeControllerWithXmlComments.ActionWithSummaryAndRemarksTags"/> | |
public new void ActionWithSummaryAndRemarksTags() | |
{ | |
throw new NotImplementedException(); | |
} | |
/// <inheritdoc cref = "FakeControllerWithXmlComments.ActionWithParamTags"/> | |
public new void ActionWithParamTags(string param1, string param2) | |
{ | |
throw new NotImplementedException(); | |
} | |
/// <inheritdoc cref = "FakeControllerWithXmlComments.ActionWithResponseTags"/> | |
/// <inheritdoc cref="FakeControllerWithXmlComments" /> | |
public class FakeControllerWithInheritDoc : FakeControllerWithXmlComments | |
{ | |
/// <inheritdoc cref="FakeControllerWithXmlComments.ActionWithSummaryAndRemarksTags"/> | |
public new void ActionWithSummaryAndRemarksTags() | |
{ | |
throw new NotImplementedException(); | |
} | |
/// <inheritdoc cref="FakeControllerWithXmlComments.ActionWithParamTags"/> | |
public new void ActionWithParamTags(string param1, string param2) | |
{ | |
throw new NotImplementedException(); | |
} | |
/// <inheritdoc cref="FakeControllerWithXmlComments.ActionWithResponseTags"/> |
namespace Swashbuckle.AspNetCore.SwaggerGen; | ||
|
||
/// <summary> | ||
/// Service to handle recursive retrieval of XML comments from an XPathNavigator. |
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.
/// Service to handle recursive retrieval of XML comments from an XPathNavigator. | |
/// Service to handle recursive retrieval of XML comments from an XPathNavigator. |
/// <summary> | ||
/// Service to handle recursive retrieval of XML comments from an XPathNavigator. | ||
/// </summary> | ||
internal static class XmlCommentsRecursionService |
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 doesn't feel "service"-y to me - it's more "helper"-y. I don't think we need recursive in the name either - that feels like an implementation detail to me.
Also the file name should match the type name.
/// An XPathNodeIterator representing the collection of nodes with the specified tag, or null if the nodes are not | ||
/// found. |
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.
/// An XPathNodeIterator representing the collection of nodes with the specified tag, or null if the nodes are not | |
/// found. | |
/// An XPathNodeIterator representing the collection of nodes with the specified tag, or null if the nodes are not | |
/// found. |
This pull request is stale because it has been open for 60 days with no activity. It will be automatically closed in 14 days if no further changes are made. |
This pull request was closed because it has been inactive for 14 days since being marked as stale. |
Fixes #2597.
This will recursively scan the XML documentation to gather the first property/attribute that contains the needed value. This change would work automatically without the user having to do anything extra.
This time, the code has been rebased and is ready to bring in. This related to PR #2687.