TypeLookUp Structured Documentation Feature to be used by VS Code #1038

akshita31 · 2017-11-30T18:37:06Z

To display the XML Documentation properly, we need omnisharp-roslyn to send a documentation object to omnisharp-vscode with different parts of the documentation in separate fields so that newlines can then be added to the documentation appropriately on the vscode side. So a V2 endpoint has been created that will be invoked by omnisharp-vscode and appropriate test cases have been added.

Edit : Instead of a V2 endpoint, a property has been added to V1 with the structuredDocumentation
Please Review @DustinCampbell @TheRealPiotrP @rchande

rchande

Overall, looking pretty good. Some style issues and possible test improvements. Thanks!

rchande · 2017-11-30T18:53:37Z

src/OmniSharp.Abstractions/Models/v2/TypeLookUp/DocumentationComment.cs

+{
+    public class DocumentationComment
+    {
+        //[DefaultValue("")]


rchande · 2017-11-30T18:53:53Z

src/OmniSharp.Abstractions/Models/v2/TypeLookUp/DocumentationComment.cs

+        public string ReturnsText { get; set; }
+        public string SummaryText { get; set; }
+        public string ValueText { get; set; }
+        public string [] Param { get; set; }


No space between type and array brackets

rchande · 2017-11-30T18:54:14Z

src/OmniSharp.Abstractions/Models/v2/TypeLookUp/DocumentationComment.cs

+
+        public DocumentationComment()
+        {
+            RemarksText = "";


Do we need to initialize the arrays to empty arrays, or does json.net not care?

rchande · 2017-11-30T18:54:41Z

src/OmniSharp.Abstractions/Models/v2/TypeLookUp/TypeLookUpRequest.cs

@@ -0,0 +1,11 @@
+using OmniSharp.Mef;
+
+namespace OmniSharp.Models.v2.TypeLookUp


Is it upper or lowercase "v" in the rest of the project?

rchande · 2017-11-30T18:54:53Z

src/OmniSharp.Abstractions/Models/v2/TypeLookUp/TypeLookUpRequest.cs

+namespace OmniSharp.Models.v2.TypeLookUp
+{
+    [OmniSharpEndpoint(OmniSharpEndpoints.V2.TypeLookup, typeof(TypeLookupRequest), typeof(TypeLookupResponse))]
+    public class TypeLookupRequest:Request


Space on both sides of the colon

rchande · 2017-11-30T19:03:53Z

src/OmniSharp.Roslyn.CSharp/Services/Types/V2/TypeLookUp.cs

+                var symbol = await SymbolFinder.FindSymbolAtPositionAsync(semanticModel, position, _workspace);
+                if (symbol != null)
+                {
+                    response.Type = symbol.Kind == SymbolKind.NamedType ?


How much of this is duplicated from the v1 endpoint? Wondering if there's a refactoring we can do here.

rchande · 2017-11-30T19:05:39Z

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

+            var expected =
+            @"gameObject: The game object.";
+            Assert.Equal(2,response.DocComment.Param.Length);
+            Assert.Equal(expected, response.DocComment.Param[0]);


You can combine this test and the one below it and check both parameters.

rchande · 2017-11-30T19:06:01Z

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

+            var expected =
+            @"T: The element type of the array";
+            Assert.Single(response.DocComment.TypeParam);
+            Assert.Equal(expected, response.DocComment.TypeParam[0]);


Can you verify it works for more than 1 type parameter?

rchande · 2017-11-30T19:06:25Z

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

+}
+";
+            var response = await GetTypeLookUpResponse(content);
+            var expected =


Why isn't the summary text in the response?

rchande · 2017-11-30T19:09:03Z

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

+            Assert.Equal(expected.Replace("\r",""), response.DocComment.SummaryText);
+        }
+
+    }


I like that you have tests for each type of section or tag, but there aren't many tests that validate that multiple sections all show up correctly together? Could you add some tests that verify what happens if you have a <summary>, <params> and <returns> all in one doc comment (for example).

Can you also add tests that validate the behavior if elements are interleaved with other elements? eg... ... ...`

filipw · 2017-11-30T19:39:26Z

The entire new handler is really a copy of the old one, except for the one line that computes documentation.
Wouldn't it be simpler to just extend the old endpoint with something like WantStructuredDocumentation on the request object and just use that?

DustinCampbell · 2017-11-30T21:49:04Z

Is this new endpoint actually necessary? Can't these properties (or a property returning a richer object) by returned by the v1 endpoint?

rchande · 2017-11-30T22:00:39Z

I think @TheRealPiotrP wanted us to start being more diligent about versioning endpoints.

DustinCampbell · 2017-12-01T16:59:25Z

@rchande : That seems reasonable, but it's a change in approach that the OmniSharp community has not taken in the past. I don't think copying and pasting entire endpoints to add a property is worthwhile. 😄 @david-driscoll, how do you feel about this?

rchande · 2017-12-01T17:00:25Z

@DustinCampbell, there's definitely a refactoring that needs to happen. I think @akshita31 is working on it.

DustinCampbell · 2017-12-01T17:03:07Z

FWIW, I'm totally comfortable adding new V2 end points if they make sense. However, general policy decisions around versioning of the protocol need to happen with the OmniSharp community.

filipw · 2017-12-01T17:21:00Z

While proper versioning is always welcome, at the same time don't think it's sustainable to have a new version with every new property being added - it only would make sense for breaking changes.

Since this (structured documentation) doesn't have to replace the old documentation property and therefore can be seen as an additive and non-breaking change, IMHO it should just be added to the existing endpoint and the complexity of the PR drastically reduced.

rchande · 2017-12-01T17:31:14Z

Fair enough, I don't have super strong feelings about this one way or the other.

david-driscoll · 2017-12-01T18:29:11Z

Yeah, if we can get by using a new property instead of fracturing endpoints it would be preferable. I'm fine making breaking changes when required, because I want to avoid causing problems for consumers that may be integrating slower, or can't/won't make use of the new endpoint.

rchande

Looks good. Thanks!

rchande · 2017-12-05T23:49:14Z

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

+                    return null;
+                }
+
+                docComment.RemarksText = remarksText.ToString();


How do you feel about adding a constructor to docComment that takes all of these, rather than having to remember to set all of the properties? (Maybe makes it easier to not forget a section if we refactor).

…comment class itself

akshita31 added 9 commits November 17, 2017 18:15

Added Basic Structure

0de6d84

Text Fields for Doc Convertor running

a6933f2

see and seeref working

02e7a35

All Test Cases Running

4b50e4e

Cleaned the code

b35485d

Removed ParamRef

9ca970f

Added test

056aed5

Converted from stringBuilder to string

5013278

Added array for exceptions

eb79ac8

rchande suggested changes Nov 30, 2017

View reviewed changes

akshita31 added 2 commits November 30, 2017 12:01

Unncessary Lines Removed and Tests Modified

c3d1faf

Added tests for tags appearing together

93b3846

akshita31 force-pushed the typelookup_v2 branch from 67df3f8 to 884742f Compare December 1, 2017 19:14

Added property to V1

a6b1a97

akshita31 force-pushed the typelookup_v2 branch from 884742f to a6b1a97 Compare December 1, 2017 19:17

rchande approved these changes Dec 5, 2017

View reviewed changes

Added the code for forming the comment object into the documentation …

90a236f

…comment class itself

DustinCampbell approved these changes Dec 7, 2017

View reviewed changes

akshita31 added 2 commits December 7, 2017 17:06

Merge remote-tracking branch 'upstream/master' into typelookup_v2

d3de7c2

Changed order of the members in DocumentationComment object

18fe9c5

akshita31 changed the title ~~V2 TypeLookUp Endpoint to be used by VS Code~~ TypeLookUp Structured Documentation Feature to be used by VS Code Dec 8, 2017

Merge branch 'master' into typelookup_v2

5494fb9

DustinCampbell merged commit aadc234 into OmniSharp:master Dec 8, 2017

akshita31 mentioned this pull request Dec 11, 2017

Proper Newlines in Hover dotnet/vscode-csharp#1918

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TypeLookUp Structured Documentation Feature to be used by VS Code #1038

TypeLookUp Structured Documentation Feature to be used by VS Code #1038

akshita31 commented Nov 30, 2017 •

edited

Loading

rchande left a comment

rchande Nov 30, 2017

rchande Nov 30, 2017

rchande Nov 30, 2017

rchande Nov 30, 2017

rchande Nov 30, 2017

rchande Nov 30, 2017

rchande Nov 30, 2017

rchande Nov 30, 2017

rchande Nov 30, 2017

rchande Nov 30, 2017 •

edited

Loading

filipw commented Nov 30, 2017

DustinCampbell commented Nov 30, 2017

rchande commented Nov 30, 2017

DustinCampbell commented Dec 1, 2017

rchande commented Dec 1, 2017

DustinCampbell commented Dec 1, 2017

filipw commented Dec 1, 2017

rchande commented Dec 1, 2017

david-driscoll commented Dec 1, 2017

rchande left a comment

rchande Dec 5, 2017

		@@ -0,0 +1,11 @@
		using OmniSharp.Mef;

		namespace OmniSharp.Models.v2.TypeLookUp

TypeLookUp Structured Documentation Feature to be used by VS Code #1038

TypeLookUp Structured Documentation Feature to be used by VS Code #1038

Conversation

akshita31 commented Nov 30, 2017 • edited Loading

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

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 Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

filipw commented Nov 30, 2017

DustinCampbell commented Nov 30, 2017

rchande commented Nov 30, 2017

DustinCampbell commented Dec 1, 2017

rchande commented Dec 1, 2017

DustinCampbell commented Dec 1, 2017

filipw commented Dec 1, 2017

rchande commented Dec 1, 2017

david-driscoll commented Dec 1, 2017

rchande left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshita31 commented Nov 30, 2017 •

edited

Loading

rchande Nov 30, 2017 •

edited

Loading