Skip to content

Conversation

@davidwengier
Copy link
Member

@davidwengier davidwengier commented Jul 24, 2020

Fixes AB#1155667

Adds support to LSP for inserting doc comments during typing, both when typing '///' above a method/class, or when pressing enter within a doc comment.

You'll pretty much have to review commit-by-commit, since the first commit is a straight cut and paste of code into a service without modification. Clean up comes later to make reviewing it easier.

@davidwengier davidwengier requested a review from a team as a code owner July 24, 2020 10:47
@jinujoseph jinujoseph added Area-IDE LSP issues related to the roslyn language server protocol implementation labels Jul 24, 2020
Copy link
Member

@dibarbet dibarbet left a comment

Choose a reason for hiding this comment

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

mostly looks good - if there are any areas in the doc comment service where you made material logic changes instead of mostly copy paste let me know

Copy link
Contributor

@allisonchou allisonchou left a comment

Choose a reason for hiding this comment

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

LGTM 🎉 (though I'd probably get another approval in addition just to be sure) All of my comments are nits, although I didn't look too closely at the move to service since I'm assuming it's just code being moved over.

@davidwengier
Copy link
Member Author

if there are any areas in the doc comment service where you made material logic changes instead of mostly copy paste let me know

The only change of note with the existing code is that in the command handler, two of the operations did an Insert and two did a Replace. Now they all do a Replace so there was some tweaking of the spans to get that to work, but the existing command handler tests all pass without modification so I'm pretty confident :)

@davidwengier
Copy link
Member Author

All of the PR feedback has been addressed

@davidwengier davidwengier merged commit 61dd560 into dotnet:master Jul 28, 2020
@ghost ghost added this to the Next milestone Jul 28, 2020
@davidwengier davidwengier deleted the CommentsAutoCompleteInLSP branch July 28, 2020 07:52
@RikkiGibson RikkiGibson modified the milestones: Next, 16.8.P2 Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-IDE LSP issues related to the roslyn language server protocol implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants