-
Notifications
You must be signed in to change notification settings - Fork 509
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
Disable documentation analysis for <exclude> elements #3121
base: master
Are you sure you want to change the base?
Conversation
// This documentation rule is excluded via the <exclude /> tag | ||
/*if (completeDocumentation.Nodes().OfType<XElement>().Any(element => element.Name == XmlCommentHelper.ExcludeXmlTag)) | ||
{ | ||
return; | ||
}*/ | ||
|
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.
More commented code
/*if (fileHeader.GetElement("exclude") != null) | ||
{ | ||
return; | ||
}*/ |
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.
Why commented? Anyway, you don't need to modify this code. The "file header" is not true XML documentation, and can't have an "exclude" tag. See https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1633.md
/*if (completeDocumentation.Nodes().OfType<XElement>().Any(element => element.Name == XmlCommentHelper.ExcludeXmlTag)) | ||
{ | ||
return; | ||
}*/ | ||
|
||
// var hasExcludeTag = completeDocumentation.Nodes().OfType<XElement>().Any(element => element.Name == XmlCommentHelper.ExcludeXmlTag); |
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.
What's up with this commented code?
Codecov Report
@@ Coverage Diff @@
## master #3121 +/- ##
==========================================
- Coverage 97.13% 97.10% -0.03%
==========================================
Files 828 828
Lines 103971 104551 +580
Branches 3354 3389 +35
==========================================
+ Hits 100991 101524 +533
- Misses 2090 2136 +46
- Partials 890 891 +1 |
public class ClassName | ||
{ | ||
/// <exclude/> | ||
public void Test() { } |
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.
❔ Should we also disable checks for a member if the containing type is marked with <exclude/>
?
📝 The test for this could be added without duplicating all of the current tests, by having a single class with several members.
/// <exclude/>
public class ClassName
{
public void ClassName() { }
~ClassName() { }
public int Property { get; set; }
public void Test() { }
// ...
}
public async Task TestTypeWithExcludedDocumentationAsync(string typeName) | ||
{ | ||
var testCode = @" | ||
/// <exclude/> |
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.
💭 It seems like the <exclude/>
element shouldn't always exclude the element, but rather it should force a public element to be treated like a non-public element. If documentation is required for non-public elements, diagnostics should still be reported for missing documentation.
I have a fix for issue #3094 as reported in the main repository, concerning the exclusion of rule enforcement for Doc rules when
<exclude/>
is present. I have created accompanying new unit tests in the StyleCop.Analyzers.Test project, all of which have passed.