Using StructuredDocumentation for Signature Help and appropriate documentation for Parameter symbols #1085

akshita31 · 2018-01-12T00:55:59Z

Changes:

Added StructuredDocumentation field to SignatureHelpResponse to display signature help documentation properly.
Added code for returning documentation for parameters, typeparameters and alias types, by adding a class DocumentationHelper and the corresponding methods.
Modified DocumentationComment to return empty object instead of null if the xmlDocumentation is empty.
Modified the GetStructuredDocumentation - to be used by both TypeLookUp and SignatureHelp to check for the type of symbol and return the DocumentationComment object accordingly.
Modified SignatureHelp to use the StructuredDocumentation for the signature and get the correct documentation for the parameter.
Modified TypeLookUp according to the changes.
Added test cases for the SignatureHelp and TypeLookUp.

Please review : @DustinCampbell @rchande @TheRealPiotrP @david-driscoll @filipw

akshita31 · 2018-01-12T01:09:48Z

With this change the Hover in VSCode will show documentation for parameters :

akshita31 · 2018-01-12T01:33:45Z

Omnisharp-vscode side : dotnet/vscode-csharp#1958

akshita31 · 2018-01-12T01:35:43Z

src/OmniSharp.Roslyn.CSharp/Services/Signatures/SignatureHelpService.cs

@@ -168,19 +169,19 @@ private static SignatureHelpItem BuildSignature(IMethodSymbol symbol)
            signature.Documentation = symbol.GetDocumentationCommentXml();


@rchande @DustinCampbell Do we want to return the Parsed xml for other editors as well. Currently GetDocumentationCommentXml() gives the xml with the tags.

rchande · 2018-01-12T16:47:35Z

src/OmniSharp.Abstractions/Models/v1/TypeLookup/DocumentationComment.cs

@@ -30,8 +30,22 @@ private DocumentationComment(string summaryText, DocumentationItem[] typeParamEl
            Exception = exception;
        }

+        private DocumentationComment()


You could make a this() constructor call and call your existing constructor with the default values (to avoid having to write the assignments over and over again).
Alternatively, you could remove this constructor and make all the parameters optional in the one above this.

rchande · 2018-01-12T16:47:52Z

src/OmniSharp.Abstractions/Models/v1/TypeLookup/DocumentationComment.cs

-        public DocumentationItem[] Exception { get; }
-
-        private DocumentationComment(string summaryText, DocumentationItem[] typeParamElements, DocumentationItem[] paramElements, string returnsText, string remarksText, string exampleText, string valueText, DocumentationItem[ ] exception)
+        public string SummaryText { get; private set; }


Why setters? You should be able to set get-only in the constructor.

rchande · 2018-01-12T16:48:07Z

src/OmniSharp.Abstractions/Models/v1/TypeLookup/DocumentationComment.cs

        public static DocumentationComment From(string xmlDocumentation, string lineEnding)
        {
+            if (string.IsNullOrEmpty(xmlDocumentation))
+                return Empty;


rchande · 2018-01-12T16:50:45Z

src/OmniSharp.Abstractions/Models/v1/TypeLookup/DocumentationComment.cs

+        //Gets the parameter documentation from this object
+        public string GetParameterText(string name)
+        {
+            var requiredParam = Array.Find(ParamElements, parameter => parameter.Name == name);


You could do an expression body here:
`GetParameterText() => Array.Find(...)?.Documentation ?? string.Empty.

rchande · 2018-01-12T16:51:25Z

src/OmniSharp.Abstractions/Models/v1/TypeLookup/DocumentationComment.cs

+
+        public static DocumentationComment WithSummaryText(string summaryText)
+        {
+            var docComment = new DocumentationComment();


Yeah, let's just make the arguments to the first constructor optional.

rchande · 2018-01-12T16:51:58Z

src/OmniSharp.Roslyn.CSharp/Services/DocumentationConverter.cs

+        {
+            switch (symbol)
+            {
+                case IParameterSymbol parameter: return DocumentationComment.WithSummaryText(DocumentationHelper.GetParameterDocumentation(parameter, lineEnding));


Typically the case code goes on the line after the case label.

Wouldn't this be nicer if it was New DocumentationComment(summaryText = DocumentationHelper.GetParameterer(...?

rchande · 2018-01-12T16:54:37Z

src/OmniSharp.Roslyn.CSharp/Services/DocumentationConverter.cs

+
+    public static class DocumentationHelper
+    {
+        public static string GetParameterDocumentation(IParameterSymbol parameter, string lineEnding = "\n")


What's the advantage of putting these in another class versus inside DocumentationConverter? Do you imagine other code using these methods?

NVM, used below. Can you move this class to its own file?

rchande · 2018-01-12T16:58:03Z

tests/OmniSharp.Roslyn.CSharp.Tests/SignatureHelpFacts.cs

+            var signature = actual.Signatures.ElementAt(0);
+            Assert.Equal(2, signature.Parameters.Count());
+            Assert.Equal("gameObject", signature.Parameters.ElementAt(0).Name);
+            Assert.Equal("The game object.", signature.Parameters.ElementAt(0).Documentation);


What happens if there are additional tags inside the parameter documentation?

The StructuredDocumentation removes all the tags and appends the text to the appropriate section hence any additional tags inside the parameter documentation will be processed accordingly.
Added a test to check for the same.

rchande · 2018-01-12T17:00:29Z

src/OmniSharp.Roslyn.CSharp/Services/Signatures/SignatureHelpService.cs

@@ -168,19 +169,19 @@ private static SignatureHelpItem BuildSignature(IMethodSymbol symbol)
            signature.Documentation = symbol.GetDocumentationCommentXml();
            signature.Name = symbol.MethodKind == MethodKind.Constructor ? symbol.ContainingType.Name : symbol.Name;
            signature.Label = symbol.ToDisplayString(SymbolDisplayFormat.MinimallyQualifiedFormat);
+            signature.StructuredDocumentation = DocumentationConverter.GetStructuredDocumentation(symbol);


It looks like this code is calling symbol.GetDocumentationCommentXml once for the method itself and once for every parameter in the method. Is there a way we can just parse the XML once?

rchande

Very close, just a couple more things.

rchande · 2018-01-12T21:30:49Z

src/OmniSharp.Abstractions/Models/v1/TypeLookup/DocumentationComment.cs

@@ -175,6 +186,15 @@ private static string TrimStartRetainingSingleLeadingSpace(string input)
                return input;
            return $" {input.TrimStart()}";
        }
+
+        //Gets the parameter documentation from this object


Make this a doc comment or remove?

rchande · 2018-01-12T21:31:28Z

src/OmniSharp.Abstractions/Models/v1/TypeLookup/DocumentationComment.cs

@@ -18,20 +18,31 @@ public class DocumentationComment
        public string ValueText { get; }
        public DocumentationItem[] Exception { get; }

-        private DocumentationComment(string summaryText, DocumentationItem[] typeParamElements, DocumentationItem[] paramElements, string returnsText, string remarksText, string exampleText, string valueText, DocumentationItem[ ] exception)
+        public DocumentationComment
+            (string summaryText = "",


Nit: paren should be on the same line as DocumentationComment

rchande · 2018-01-12T21:32:02Z

src/OmniSharp.Abstractions/Models/v1/TypeLookup/DocumentationComment.cs

        {
            SummaryText = summaryText;
-            TypeParamElements = typeParamElements;
-            ParamElements = paramElements;
+            TypeParamElements = typeParamElements ?? new DocumentationItem[0];


Array.Empty

rchande · 2018-01-12T21:33:05Z

src/OmniSharp.Roslyn.CSharp/Services/DocumentationHelper.cs

+        public static string GetParameterDocumentation(IParameterSymbol parameter, string lineEnding = "\n")
+        {
+            var contaningSymbolDef = parameter.ContainingSymbol.OriginalDefinition;
+            return DocumentationConverter.GetStructuredDocumentation(contaningSymbolDef.GetDocumentationCommentXml(), lineEnding).GetParameterText(parameter.Name);


I would wrap a line of this length.

rchande · 2018-01-12T21:35:10Z

src/OmniSharp.Roslyn.CSharp/Services/Signatures/SignatureHelpService.cs


            signature.Parameters = symbol.Parameters.Select(parameter =>
            {
                return new SignatureHelpParameter()
                {
                    Name = parameter.Name,
                    Label = parameter.ToDisplayString(SymbolDisplayFormat.MinimallyQualifiedFormat),
-                    Documentation = parameter.GetDocumentationCommentXml()
+                    Documentation = signature.StructuredDocumentation.GetParameterText(parameter.Name)


Ah, now there's only one caller for each of those methods in DocumentationHelper. Can they move inside StructuredDocumentation?

rchande

Thanks!

akshita31 added 7 commits January 10, 2018 13:10

Structured Documentation in Signature Help

d9da163

Changes made to reflect only in o#-vscode and not in other editors

c3ca7b9

Return documentation for parameter

82621ab

Structured Documentation returning empty object instead of null

b045909

Merge Conflicts Resolved

e1fc79f

Typelookup and Signature Help running for parameters

021226a

Remove unnecessary using

d936717

akshita31 changed the title ~~Using StructuredDocumentation for Signature Help and Documentation for Parameter symbols~~ Using StructuredDocumentation for Signature Help and appropriate documentation for Parameter symbols Jan 12, 2018

akshita31 mentioned this pull request Jan 12, 2018

Using Structured Documentation for Signature Help and improving Parameter Documentation dotnet/vscode-csharp#1958

Merged

akshita31 commented Jan 12, 2018

View reviewed changes

rchande suggested changes Jan 12, 2018

View reviewed changes

akshita31 added 4 commits January 12, 2018 11:38

Made parameters in constructor optional

9a56685

Parsing XML only once for a signature

2eeaefa

Test for nested tags in parameter documentation

2b948c2

Removed unnecessary using

54e7066

rchande suggested changes Jan 12, 2018

View reviewed changes

Moved methods inside documentation convertor

0239199

rchande approved these changes Jan 12, 2018

View reviewed changes

akshita31 and others added 2 commits January 22, 2018 11:20

Merge branch 'master' into sig_help_doc

5b402b2

Merge branch 'master' into sig_help_doc

43202ef

rchande merged commit ab289f5 into OmniSharp:master Jan 29, 2018

akshita31 deleted the sig_help_doc branch January 29, 2018 23:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using StructuredDocumentation for Signature Help and appropriate documentation for Parameter symbols #1085

Using StructuredDocumentation for Signature Help and appropriate documentation for Parameter symbols #1085

akshita31 commented Jan 12, 2018 •

edited

Loading

akshita31 commented Jan 12, 2018

akshita31 commented Jan 12, 2018

akshita31 Jan 12, 2018

rchande Jan 12, 2018

rchande Jan 12, 2018

rchande Jan 12, 2018

rchande Jan 12, 2018

rchande Jan 12, 2018

rchande Jan 12, 2018

rchande Jan 12, 2018

rchande Jan 12, 2018

rchande Jan 12, 2018

rchande Jan 12, 2018

akshita31 Jan 12, 2018

rchande Jan 12, 2018

rchande left a comment

rchande Jan 12, 2018

rchande Jan 12, 2018

rchande Jan 12, 2018

rchande Jan 12, 2018

rchande Jan 12, 2018

rchande left a comment

		@@ -168,19 +169,19 @@ private static SignatureHelpItem BuildSignature(IMethodSymbol symbol)
		signature.Documentation = symbol.GetDocumentationCommentXml();

Using StructuredDocumentation for Signature Help and appropriate documentation for Parameter symbols #1085

Using StructuredDocumentation for Signature Help and appropriate documentation for Parameter symbols #1085

Conversation

akshita31 commented Jan 12, 2018 • edited Loading

akshita31 commented Jan 12, 2018

akshita31 commented Jan 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchande left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchande left a comment

Choose a reason for hiding this comment

akshita31 commented Jan 12, 2018 •

edited

Loading