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

Add documentation comment creation to the FormatAfterKeystrokeService #2023

Merged
merged 4 commits into from
Dec 1, 2020

Conversation

333fred
Copy link
Contributor

@333fred 333fred commented Nov 23, 2020

IDocumentationCommentSnippetService was recently moved down to the Features layer, which means omnisharp can now take advantage of it and generate documentation comment snippets on typing. There are a couple of caveats:

  1. LSP's "/onTypingFormat" endpoint has no support for setting the cursor location after format, which means that the cursor ends up after the inserted block. This is fine for newlines inside doc comments, but awkward for the initial expansion. However, unless we want to add special support with a custom endpoint, we can't get around this (the custom endpoint is how Roslyn does it).
  2. The service itself is still internal, so I had to add a few new reflection points.

Server-side of dotnet/vscode-csharp#8. It also needs a vscode change, but it's a one-line change I'll submit later after we get this merged and I can write some integration tests.

IDocumentationCommentSnippetService was recently moved down to the Features layer, which means omnisharp can now take advantage of it and generate documentation comment snippets on typing. There are a couple of caveats:

1. LSP's "/onTypingFormat" endpoint has no support for setting the cursor location after format, which means that the cursor ends up after the inserted block. This is fine for newlines inside doc comments, but awkward for the initial expansion. However, unless we want to add special support with a custom endpoint, we can't get around this (the custom endpoint is how Roslyn does it).
2. The service itself is still internal, so I had to add a few new reflection points.

Server-side of dotnet/vscode-csharp#8. It also needs a vscode change, but it's a one-line change I'll submit later after we get this merged and I can write some integration tests.
static DocumentationCommentSnippetService()
{
var iDocumentationCommentSnippetServiceType = typeof(CompletionItem).Assembly.GetType("Microsoft.CodeAnalysis.DocumentationComments.IDocumentationCommentSnippetService");
s_getDocumentationCommentSnippetOnCharacterTyped = iDocumentationCommentSnippetServiceType.GetMethod(nameof(GetDocumentationCommentSnippetOnCharacterTyped));
Copy link
Member

Choose a reason for hiding this comment

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

I saw that there is also GetDocumentationCommentSnippetOnCommandInvoke, I think it is used in VS when you invoke "insert comment" command from insert snippet feature. we may also want to expose it in the future

Copy link
Member

@filipw filipw left a comment

Choose a reason for hiding this comment

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

thanks a lot!

@filipw
Copy link
Member

filipw commented Nov 23, 2020

I'd like to add that the current popular solution to do this in VS Code is to use k--kato.docomment extension.
There have been several times already that we had to deal with an issue report in OmniSharp (unresponsive etc) that turned out to be caused by this extension.

@filipw filipw merged commit 5a64da0 into OmniSharp:master Dec 1, 2020
@333fred 333fred deleted the auto-doc-comment branch December 1, 2020 17:11
@lonix1
Copy link

lonix1 commented Feb 11, 2021

It seems that this feature requires us to set "editor.formatOnType": true.

That is unfortunate as many people (me included) don't like it - it ruins one's coding workflow, and besides there are many formatting tools that handle that sort of thing.

Is it possible for this feature to work without that setting?

(BTW: the k--kato.docomment extension doesn't need that setting in order to expand ///.)

@333fred
Copy link
Contributor Author

333fred commented Feb 11, 2021

It seems that this feature requires us to set "editor.formatOnType": true.

That is unfortunate as many people (me included) don't like it - it ruins one's coding workflow, and besides there are many formatting tools that handle that sort of thing.

Is it possible for this feature to work without that setting?

(BTW: the k--kato.docomment extension doesn't need that setting in order to expand ///.)

No, it is not. Not and be compatible with LSP. We need a new LSP feature to make it more generally available.

@filipw
Copy link
Member

filipw commented Feb 11, 2021

also, k--kato.docomment runs the check "should I expand comment" on every text change in the editor which is very aggressive

@lonix1
Copy link

lonix1 commented Feb 12, 2021

Thank you for explaining. I didn't realize that plugin was that aggressive!

We need a new LSP feature

In vscode or omnisharp? And is there already an issue tracking this, or do you want me to open one?

@333fred
Copy link
Contributor Author

333fred commented Feb 12, 2021

In vscode or omnisharp?

Neither, in LSP. And I imagine there's an open proposal somewhere for an LSP endpoint that could accomplish this, but I haven't really looked.

@lonix1
Copy link

lonix1 commented Feb 13, 2021

@333fred Well then it's obviously beyond my understanding, so I wouldn't even know how to write the issue. 😄

I hope you'll find some time to investigate. Until then thanks for this PR, it's a decent approach for now.

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.

4 participants