Handling Newlines in XML Documentation #1869

akshita31 · 2017-11-16T01:01:16Z

Issue : #1057

The XML Documentation being returned by omnisharp-roslyn contained newlines properly, however vscode is displaying a single newline when there are two consecutive new lines are provided. Hence a change has been made to the hoverProvider to replace a single newline with two new lines. An integration test has been added for the same. However on adding this test, the "Launch singleCsproj Workspace Tests" and "Launch slnWithCsproj Workspace Tests" are failing.

Please review @DustinCampbell @TheRealPiotrP @rchande

TheRealPiotrP · 2017-11-16T03:17:52Z

test/integrationTests/hoverProvider.integration.test.ts

+            await csharpExtension.activate();
+        }
+
+        testAssetWorkspace.deleteBuildArtifacts();


do we need this?

TheRealPiotrP · 2017-11-16T03:17:57Z

test/integrationTests/hoverProvider.integration.test.ts

+
+        testAssetWorkspace.deleteBuildArtifacts();
+
+        await fs.rimraf(testAssetWorkspace.vsCodeDirectoryPath);


TheRealPiotrP · 2017-11-16T03:18:05Z

test/integrationTests/hoverProvider.integration.test.ts

+
+        await csharpExtension.exports.initializationFinished;
+
+        await vscode.commands.executeCommand("dotnet.generateAssets");


TheRealPiotrP · 2017-11-16T03:18:13Z

test/integrationTests/hoverProvider.integration.test.ts

+
+        await vscode.commands.executeCommand("dotnet.generateAssets");
+
+        await poll(async () => await fs.exists(testAssetWorkspace.launchJsonPath), 10000, 100);


TheRealPiotrP · 2017-11-16T03:19:48Z

test/integrationTests/hoverProvider.integration.test.ts

+        await vscode.commands.executeCommand("vscode.open", fileUri);
+
+        let c = await vscode.commands.executeCommand("vscode.executeHoverProvider", fileUri,new vscode.Position(10,30));
+        let answer:string = "Checks if object is tagged with the tag.\n\ngameObject: The game object.\n\ntagName: Name of the tag \n\nReturns: Returns trueif object is tagged with tag.";


the :string type specifier should not be necessary here.

TheRealPiotrP · 2017-11-16T03:20:22Z

test/integrationTests/hoverProvider.integration.test.ts

+
+        let c = await vscode.commands.executeCommand("vscode.executeHoverProvider", fileUri,new vscode.Position(10,30));
+        let answer:string = "Checks if object is tagged with the tag.\n\ngameObject: The game object.\n\ntagName: Name of the tag \n\nReturns: Returns trueif object is tagged with tag.";
+        expect(c[0].contents[0].value).to.equal(answer);


for my own education, why might contents have multiple values?

There are 2 parts to the hover: the type info, and the documentation.

TheRealPiotrP · 2017-11-16T03:22:57Z

test/integrationTests/hoverProvider.integration.test.ts

+
+    test("Hover returns the correct XML", async () => {                
+
+        let program : string = "using System;\


Can we use a multi-line string instead? it should be much easier to maintain since we won't be in the business of moving around \n's and \'s.

DustinCampbell · 2017-11-16T14:33:00Z

test/integrationTests/hoverProvider.integration.test.ts

+        expect(c[0].contents[0].value).to.equal(answer);
+
+    });
+


extra blank line

DustinCampbell · 2017-11-16T14:33:47Z

test/integrationTests/hoverProvider.integration.test.ts

@@ -0,0 +1,63 @@
+


Missing copyright header

@akshita31 if you install the tslint extension for vs code you will get notification for this and similar issues.

rchande · 2017-11-16T16:31:57Z

src/features/hoverProvider.ts

@@ -21,7 +21,8 @@ export default class OmniSharpHoverProvider extends AbstractSupport implements H

        return serverUtils.typeLookup(this._server, req, token).then(value => {
            if (value && value.Type) {
-                let contents = [extractSummaryText(value.Documentation), { language: 'csharp', value: value.Type }];
+                let addLine = value.Documentation.split("\n").join("\n\n");


I'm not convinced that this is correct. The bug wants us to have line breaks between sections (summary, parameter, etc), but this seems like it's going to add them anywhere a newline appears in the doc comment. Can you post a screenshot with your changes, with a doc comment with a multline summary section?

Here is the output on adding a newline in the summary text. Isn't this what we are expecting?

I don't think there should be a visible line break between "the" and "tag".

An interesting observation here is that even if there are multiple newlines together in the text, vscode displays it as a single newline only. But I am not really sure whether this is ok or we need to do something about it.

There are multiple "kinds" of newlines at play.

There are single newlines that have to be in doc comments if a section spans more than one line. I contend that we should not cause a visible line break for those

There might be multiple consecutive newlines in a doc comment section where the user was trying to separate paragraphs. We should make those visible.

There are visible line breaks between sections (like between summary and parameters, or between parameters). We should always show those, even if the user wrote all their parameter documentation sections on one line.

This is the output from the master branch(where no changes have been made). As noticeable, the problem is that vscode doesn't show a newline for a single newline.It shows a "single" newline for 2 or more newlines(irrespective of whether they were part of the text originally or added by roslyn to separate the tags). Hence a quick solution was to double the '\n' character before being passed to the Hover object.

If we have to deal with single newlines and double newlines already present in the text separately, we will have to make changes on the roslyn side.Is that what you are suggesting?

Yes, it looks like we will need to make a change that spans both repos. I propose that we:

Augment Roslyn to be able to return some data structure that contains all of the "sections"

Consume that on the VSCode side and properly insert newlines between sections.

@rchande can we split this up into two features? @akshita31's change here makes the text much more readable than before, though it does not replicate VS's behavior. I'd start by finishing off this PR, then making it better once we have a chance to close on and implement the relevant change in omnisharp-roslyn. Thoughts?

@TheRealPiotrP I pulled @akshita31's branch to play with the behavior. Given the doc comment in the first screenshot, this is what it looks like before and after the change, respectively. I feel that the second example is enough of a regression to the readability of longer documentation comments that we should wait until we have an appropriate change on the roslyn side. @DustinCampbell what do you think?

I agree. Let's not merge changes that regress the user experience.

rchande · 2017-11-16T16:33:40Z

test/integrationTests/hoverProvider.integration.test.ts

+
+    test("Hover returns the correct XML", async () => {                
+
+        let program : string = "using System;\


rchande · 2017-11-16T16:33:58Z

test/integrationTests/hoverProvider.integration.test.ts

+        \n{\
+        \n    class testissue\
+        \n    {\
+        \n    ///<summary>Checks if object is tagged with the tag.</summary>\


Put a newline inside this tag.

rchande · 2017-11-16T16:34:43Z

test/integrationTests/hoverProvider.integration.test.ts

+
+        let c = await vscode.commands.executeCommand("vscode.executeHoverProvider", fileUri,new vscode.Position(10,30));
+        let answer:string = "Checks if object is tagged with the tag.\n\ngameObject: The game object.\n\ntagName: Name of the tag \n\nReturns: Returns trueif object is tagged with tag.";
+        expect(c[0].contents[0].value).to.equal(answer);


There are 2 parts to the hover: the type info, and the documentation.

TheRealPiotrP · 2017-11-17T07:19:54Z

test/integrationTests/hoverProvider.integration.test.ts

+    });
+
+
+    test("Hover returns the correct XML", async () => {                


Can we make this more specific? Perhaps HoverProvider replaces \n with \n\n? Hover does not return XML, and this test is validating our newline replacement strategy.

TheRealPiotrP

One remaining comment about test name, and a question to @rchande. LGTM otherwise.

DustinCampbell · 2017-11-17T14:15:01Z

One remaining comment about test name, and a question to @rchande. LGTM otherwise.

Except that, looking at the new hover tooltip, this change doesn't solve the problem. 😄

DustinCampbell

It seems that simply replacing all \n with \n\n does not produce the right user experience.

DustinCampbell · 2017-12-12T16:58:49Z

@akshita31 : Should this be closed in favor of #1918?

akshita31 · 2017-12-12T18:14:37Z

Yes .Closing this in favor of #1918

Adding Integration Test for hoverProvider

8771df5

dnfclas added cla-required and removed cla-required labels Nov 16, 2017

TheRealPiotrP suggested changes Nov 16, 2017

View reviewed changes

DustinCampbell reviewed Nov 16, 2017

View reviewed changes

rchande suggested changes Nov 16, 2017

View reviewed changes

Removed unnecessary lines.Changed multiline

9384b74

TheRealPiotrP reviewed Nov 17, 2017

View reviewed changes

TheRealPiotrP approved these changes Nov 17, 2017

View reviewed changes

DustinCampbell requested changes Nov 17, 2017

View reviewed changes

Merge branch 'master' into addlines

cac1569

ghost removed the cla-signed label Dec 7, 2017

ghost deleted a comment from dnfclas Dec 7, 2017

akshita31 closed this Dec 12, 2017

akshita31 deleted the addlines branch February 23, 2018 23:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handling Newlines in XML Documentation #1869

Handling Newlines in XML Documentation #1869

akshita31 commented Nov 16, 2017

TheRealPiotrP Nov 16, 2017

TheRealPiotrP Nov 16, 2017

TheRealPiotrP Nov 16, 2017

TheRealPiotrP Nov 16, 2017

TheRealPiotrP Nov 16, 2017

TheRealPiotrP Nov 16, 2017

rchande Nov 16, 2017

TheRealPiotrP Nov 16, 2017

rchande Nov 16, 2017

DustinCampbell Nov 16, 2017

DustinCampbell Nov 16, 2017

TheRealPiotrP Nov 17, 2017

rchande Nov 16, 2017

akshita31 Nov 16, 2017 •

edited

Loading

rchande Nov 16, 2017

akshita31 Nov 16, 2017

rchande Nov 16, 2017 •

edited

Loading

akshita31 Nov 16, 2017

rchande Nov 16, 2017

TheRealPiotrP Nov 17, 2017

rchande Nov 17, 2017

DustinCampbell Nov 17, 2017

rchande Nov 16, 2017

rchande Nov 16, 2017

rchande Nov 16, 2017

TheRealPiotrP Nov 17, 2017

TheRealPiotrP left a comment

DustinCampbell commented Nov 17, 2017

DustinCampbell left a comment

DustinCampbell commented Dec 12, 2017

akshita31 commented Dec 12, 2017


		testAssetWorkspace.deleteBuildArtifacts();

		await fs.rimraf(testAssetWorkspace.vsCodeDirectoryPath);


		await csharpExtension.exports.initializationFinished;

		await vscode.commands.executeCommand("dotnet.generateAssets");


		await vscode.commands.executeCommand("dotnet.generateAssets");

		await poll(async () => await fs.exists(testAssetWorkspace.launchJsonPath), 10000, 100);


		test("Hover returns the correct XML", async () => {

		let program : string = "using System;\

Handling Newlines in XML Documentation #1869

Handling Newlines in XML Documentation #1869

Conversation

akshita31 commented Nov 16, 2017

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

akshita31 Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchande Nov 16, 2017 • edited Loading

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

TheRealPiotrP left a comment

Choose a reason for hiding this comment

DustinCampbell commented Nov 17, 2017

DustinCampbell left a comment

Choose a reason for hiding this comment

DustinCampbell commented Dec 12, 2017

akshita31 commented Dec 12, 2017

akshita31 Nov 16, 2017 •

edited

Loading

rchande Nov 16, 2017 •

edited

Loading