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

WIP: New completion service #884

Closed

Conversation

DustinCampbell
Copy link
Contributor

@DustinCampbell DustinCampbell commented Jun 2, 2017

This change adds new '/completion' and '/completionItem/resolve' end points for better exposing the Roslyn completion service. In order to make this work, I finally had to make the '/open' and '/close' end points work as well. In order for the workspace to properly handle open/closed states for documents, I added a SourceText wrapper for OmniSharp that allows us to control our own SourceTextContainer instances. FWIW, getting a workspace text layer right is one of the more complex parts of the Roslyn workspace design. I have a lot more I want to do here, such as take advantage of the VS text layer.

cc @Pilchie

TODO:

  • Return commit characters from the '/completion' end point
  • Return commit character modification rules for items
  • Return optional caret position
  • Tests

@filipw
Copy link
Member

filipw commented Jun 6, 2017

awesome!

I have two questions:

  • at the moment the existing autocomplete service does some custom sorting at the end, before responding (by exact match, by ignore case match, by subsequence match etc), how does the completion service sort?
  • is XML documentation included here?

@DustinCampbell
Copy link
Contributor Author

at the moment the existing autocomplete service does some custom sorting at the end, before responding (by exact match, by ignore case match, by subsequence match etc), how does the completion service sort?

The completion service returns the list already sorted as it appears in VS. Note that the list returned by the /completion end point is not filtered by the word that's already typed as the /autocomplete end point. Instead, the expectation is that the filtering happens in the client (which is what VS Code does).

is XML documentation included here?

Yes, XML documentation is returned by this list, though it is already processed. See the Description_has_minimally_qualified_crefs test I added.

}

var previousItem = previousCompletionList.Items[itemIndex];
if (!string.Equals(previousItem.DisplayText, displayText))
Copy link
Contributor

Choose a reason for hiding this comment

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

The doc comment makes it sound like setting displayText is optional, but this will always fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I had intended it to be optional. Thanks!

if (completionList == null)
{
return CompletionModels.CompletionResponse.Empty;
}

var isSuggestionMode = completionList.SuggestionModeItem != null;
Copy link
Contributor

Choose a reason for hiding this comment

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

This loses the placeholder text for the item, and I don't think there is a way to resolve the description text for the builder anymore.

Copy link
Contributor Author

@DustinCampbell DustinCampbell Jun 7, 2017

Choose a reason for hiding this comment

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

Yup. There's no notion of a "builder" in any of the editors that use OmniSharp. That's very much a VS concept. Ultimately, this is just used on the client side to handle commit characters (if the client has that concept). If OmniSharp supports a client that has a "builder", this end point could easily be augmented by including the necessary properties from the 'SuggestionModeItem', such as 'SuggestionModeDescription'.

{
Line = endTextLine.LineNumber,
Column = change.TextChange.Span.End - endTextLine.Start
};
Copy link
Contributor

Choose a reason for hiding this comment

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

A helper to go from a TextChange to a Range seems useful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do. We may have one and I just didn't spot it.

@Pilchie
Copy link
Contributor

Pilchie commented Jun 7, 2017

This looks awesome! Also @CyrusNajmabadi might want to look.

Copy link
Member

@david-driscoll david-driscoll left a comment

Choose a reason for hiding this comment

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

Every time I had a question... I continued to review and
answered it myself. 😄

This will be good with LSP... that's on my weekend project list.

@ruant
Copy link

ruant commented Oct 3, 2018

@DustinCampbell How's this thing coming along? 😃

@DustinCampbell
Copy link
Contributor Author

@ruant : It's not dead yet, but I have some tweaks that I need to make before it can be merged. Unfortunately, this is stacked up behind a few other high priority work items.

@filipw
Copy link
Member

filipw commented Aug 10, 2020

this is superseded by #1877

@filipw filipw closed this Aug 10, 2020
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.

5 participants