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

Undocumented public API metric should count only documentation comments #2148

Merged
merged 2 commits into from
Dec 13, 2018

Conversation

valhristov
Copy link
Contributor

@valhristov valhristov commented Dec 11, 2018

Fix #2144

@ghost ghost assigned valhristov Dec 11, 2018
@ghost ghost added the Status: Needs Review label Dec 11, 2018
{
case SyntaxKind.SingleLineCommentTrivia:
case SyntaxKind.MultiLineCommentTrivia:
return trivia.ToString().TrimStart().StartsWith("///");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Roslyn does not recognize the C# "documentation" comment trivia as such unless you specify "/p:DocumentationFile=foo.xml" as command line argument

Copy link
Contributor

Choose a reason for hiding this comment

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

This comment should be put in the code.

{
case SyntaxKind.DocumentationCommentExteriorTrivia:
case SyntaxKind.DocumentationCommentTrivia:
return true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like Roslyn always recognizes documentation comments for VB.NET

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment should be in the code (reworded).

protected override IEnumerable<SyntaxNode> PublicApiNodes =>
publicApis.Value;
protected override ImmutableArray<SyntaxNode> PublicApiNodes =>
publicApiNodes.Value;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renaming to be consistent with C#

@@ -296,20 +296,18 @@ public void linesOfCodeByLine() {

assertThat(value).contains("9=1");
assertThat(value).contains("10=1");

assertThat(value).contains("11=1");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The lines of code shifted because of the new comments I added

@@ -25,7 +27,7 @@ class Class1
public void Main()
{
MyClazz myClazz = new MyClazz();
Console.WriteLine("Hello, world! " + myClazz.MyProperty);
Console.WriteLine(myClazz.MyProperty);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot to revert this change I made while debugging...

publicNodes.Add(member);
}

if (!isPublic &&
Copy link
Contributor

Choose a reason for hiding this comment

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

You could do else if (!member.IsKind(SyntaxKind.NamespaceDeclaration)) to avoid repeating the !isPublic that is already tested previously.

switch (trivia.Kind())
{
case SyntaxKind.SingleLineCommentTrivia:
case SyntaxKind.MultiLineCommentTrivia:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why we check MultiLineCommentTrivia as we check /// right after.

{
case SyntaxKind.SingleLineCommentTrivia:
case SyntaxKind.MultiLineCommentTrivia:
return trivia.ToString().TrimStart().StartsWith("///");
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment should be put in the code.

{
case SyntaxKind.DocumentationCommentExteriorTrivia:
case SyntaxKind.DocumentationCommentTrivia:
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment should be in the code (reworded).

@Evangelink Evangelink merged commit 3ef3781 into master Dec 13, 2018
@Evangelink Evangelink deleted the val/undocumented-api branch December 13, 2018 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants