-
Notifications
You must be signed in to change notification settings - Fork 684
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
Handling Newlines in XML Documentation #1869
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing copyright header There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @akshita31 if you install the |
||
import * as fs from 'async-file'; | ||
import * as vscode from 'vscode'; | ||
import * as path from 'path'; | ||
|
||
import poll from './poll'; | ||
import { should, expect } from 'chai'; | ||
import testAssetWorkspace from './testAssets/testAssetWorkspace'; | ||
|
||
const chai = require('chai'); | ||
chai.use(require('chai-arrays')); | ||
chai.use(require('chai-fs')); | ||
|
||
suite(`Test Hover Behavior ${testAssetWorkspace.description}`, function() { | ||
suiteSetup(async function() { | ||
should(); | ||
|
||
let csharpExtension = vscode.extensions.getExtension("ms-vscode.csharp"); | ||
if (!csharpExtension.isActive) { | ||
await csharpExtension.activate(); | ||
} | ||
|
||
testAssetWorkspace.deleteBuildArtifacts(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need this? |
||
|
||
await fs.rimraf(testAssetWorkspace.vsCodeDirectoryPath); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or this? |
||
|
||
await csharpExtension.exports.initializationFinished; | ||
|
||
await vscode.commands.executeCommand("dotnet.generateAssets"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or this? |
||
|
||
await poll(async () => await fs.exists(testAssetWorkspace.launchJsonPath), 10000, 100); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or this? |
||
|
||
}); | ||
|
||
|
||
test("Hover returns the correct XML", async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we make this more specific? Perhaps |
||
|
||
let program : string = "using System;\ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 |
||
\nnamespace hoverXmlDoc \ | ||
\n{\ | ||
\n class testissue\ | ||
\n {\ | ||
\n ///<summary>Checks if object is tagged with the tag.</summary>\ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Put a newline inside this tag. |
||
\n /// <param name=\"gameObject\">The game object.</param> \ | ||
\n /// <param name=\"tagName\">Name of the tag </param>\ | ||
\n /// <returns>Returns <c> true</c>if object is tagged with tag.</returns>\ | ||
\n\ | ||
\n public static bool Compare(int gameObject,string tagName)\ | ||
\n {\ | ||
\n return gameObject.TagifyCompareTag(tagName);\ | ||
\n }\ | ||
\n }\ | ||
}"; | ||
let fileUri = await testAssetWorkspace.projects[0].addFileWithContents("test.cs", program); | ||
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."; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the |
||
expect(c[0].contents[0].value).to.equal(answer); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for my own education, why might There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are 2 parts to the hover: the type info, and the documentation. |
||
|
||
}); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. extra blank line |
||
}); |
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.
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?
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.
Here is the output on adding a newline in the summary text. Isn't this what we are expecting?
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.
I don't think there should be a visible line break between "the" and "tag".
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.
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 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.
There are multiple "kinds" of newlines at play.
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.
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?
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.
Yes, it looks like we will need to make a change that spans both repos. I propose that we:
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.
@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?
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.
@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?
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.
I agree. Let's not merge changes that regress the user experience.