-
Notifications
You must be signed in to change notification settings - Fork 327
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
Inline completions #1190
Inline completions #1190
Conversation
Thanks a lot for the PR. Looks overall very good. |
|
||
const inlineCompletionItem = new code.InlineCompletionItem(insertText, asRange(item.range), command); | ||
|
||
if (item.filterText) {inlineCompletionItem.filterText = item.filterText;} |
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.
Minor :-). Missing space
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.
Done.
@@ -23,7 +23,7 @@ import semverSatisfies = require('semver/functions/satisfies'); | |||
export * from 'vscode-languageserver-protocol/node'; | |||
export * from '../common/api'; | |||
|
|||
const REQUIRED_VSCODE_VERSION = '^1.67.0'; // do not change format, updated by `updateVSCode` script | |||
const REQUIRED_VSCODE_VERSION = '^1.68.0'; // do not change format, updated by `updateVSCode` script |
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 normally updated via a tool.
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 used the tool but think I discarded some changes. Re-ran it.
client/package.json
Outdated
@@ -1,11 +1,11 @@ | |||
{ | |||
"name": "vscode-languageclient", | |||
"description": "VSCode Language client implementation", | |||
"version": "8.1.0-next.6", | |||
"version": "8.1.0-next.7", |
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 released 8.1.0 yesterday. So you need to rebase and move to 8.2.0-next.1
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.
Done.
protocol/src/common/protocol.ts
Outdated
@@ -121,6 +121,8 @@ import { | |||
DidCloseNotebookDocumentNotification | |||
} from './protocol.notebook'; | |||
|
|||
import {InlineCompletionClientCapabilities, InlineCompletionOptions, InlineCompletionParams, InlineCompletionRegistrationOptions, InlineCompletionRequest} from './protocol.inlineCompletion'; |
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.
Space
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.
Done.
types/src/main.ts
Outdated
/** | ||
* The format of the insert text. The format applies to the `insertText`. If omitted defaults to `InsertTextFormat.PlainText`. | ||
*/ | ||
insertTextFormat?: InsertTextFormat; |
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 would move this up under insertText.
We could also start thinking about introducing a complex string type that encapsulates this. E.g.
type StringContent = {
kind: 'string' | 'snippet';
value: string;
}
The reason why we didn't do that for completion items was backwards compatibility.
@aeschli do you have thoghts?
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.
Moved. Happy to alter this to use the complex type instead.
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.
@aeschli friendly ping. I am actually leaning more towards adding the StringContent
type.
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, agree, makes sense to avoid the additional property as don't need to be backward compatible.
In vscode.d.ts
there's the SnippetString
.
So I suggest: insertText: string | SnippetString;
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.
Added the SnippetString type, removed the InsertTextFormat usage, and updated the Inline Completions to use the new type.
types/src/main.ts
Outdated
|
||
export namespace InlineCompletionItem { | ||
export function create(insertText: string, filterText?: string, range?: Range, command?: Command, insertTextFormat?: InsertTextFormat): InlineCompletionItem { | ||
return {insertText, filterText, range, command, insertTextFormat}; |
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.
spacing
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.
Done.
types/src/main.ts
Outdated
|
||
export namespace InlineCompletionList { | ||
export function create(items: InlineCompletionItem[]): InlineCompletionList { | ||
return {items}; |
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.
spacing
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.
Done.
types/src/main.ts
Outdated
|
||
export namespace SelectedCompletionInfo { | ||
export function create(range: Range, text: string): SelectedCompletionInfo { | ||
return {range, text}; |
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.
spacing
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.
Done.
types/src/main.ts
Outdated
|
||
export namespace InlineCompletionContext{ | ||
export function create(triggerKind: InlineCompletionTriggerKind, selectedCompletionInfo?: SelectedCompletionInfo): InlineCompletionContext { | ||
return {triggerKind, selectedCompletionInfo}; |
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.
spacing
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.
Done.
Updated metaModel.json via the script |
de30cca
to
7e551fe
Compare
The version number of types and protocol need to be moved to |
client/src/common/client.ts
Outdated
@@ -1739,6 +1742,7 @@ export abstract class BaseLanguageClient implements FeatureClient<Middleware, La | |||
this.registerFeature(new InlayHintsFeature(this)); | |||
this.registerFeature(new DiagnosticFeature(this)); | |||
this.registerFeature(new NotebookDocumentSyncFeature(this)); | |||
this.registerFeature(new InlineCompletionItemFeature(this)); |
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.
Actually I overlooked that in the first review. We in general first add new features as proposed even if they are already final in VS Code since we need to allow the LSP community to provide feedback as well.
So we need to add the feater first in ProposedFeatures.createAll
and a client needs to call registerProposedFeatures
to make use of 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.
Sorry for not noticing this earlier.
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.
No worries. Done.
server/src/common/server.ts
Outdated
export type Languages = _Languages & CallHierarchy & SemanticTokensFeatureShape & LinkedEditingRangeFeatureShape & TypeHierarchyFeatureShape & InlineValueFeatureShape & InlayHintFeatureShape & DiagnosticFeatureShape & MonikerFeatureShape; | ||
const LanguagesImpl: new () => Languages = MonikerFeature(DiagnosticFeature(InlayHintFeature(InlineValueFeature(TypeHierarchyFeature(LinkedEditingRangeFeature(SemanticTokensFeature(CallHierarchyFeature(_LanguagesImpl)))))))) as (new () => Languages); | ||
export type Languages = _Languages & CallHierarchy & SemanticTokensFeatureShape & LinkedEditingRangeFeatureShape & TypeHierarchyFeatureShape & InlineValueFeatureShape & InlayHintFeatureShape & DiagnosticFeatureShape & MonikerFeatureShape & InlineCompletionFeatureShape; | ||
const LanguagesImpl: new () => Languages = InlineCompletionFeature(MonikerFeature(DiagnosticFeature(InlayHintFeature(InlineValueFeature(TypeHierarchyFeature(LinkedEditingRangeFeature(SemanticTokensFeature(CallHierarchyFeature(_LanguagesImpl))))))))) as (new () => Languages); |
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.
Same as with the client. The InlineCompletionFeature first must come as a proposed feature users need to opt in in the beginning. Registering a propsed feature on the server is a little bit more tricky and I can help with this. An example can be found here: https://github.com/microsoft/vscode-languageserver-node/blob/9dd23aa6d06cada41964f972021924503929bfdd/server/src/common/api.ts
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 think I did this correctly but the need to cast createConnection
in testServer.ts gave me some pause. Let me know if it should be adjusted further.
66c1eb2
to
e7db9b9
Compare
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.
Could you please do these two changes.
types/src/main.ts
Outdated
* | ||
* @since 3.18.0 | ||
*/ | ||
export interface SnippetString { |
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.
To make this easier to extend for the future we should have something more like
export interface StringValue {
kind: 'snippet';
value: string;
}
Otherwise it is very hard to evolve in the future.
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.
Done.
e7db9b9
to
5167b24
Compare
|
||
/** | ||
* Represents a collection of {@link InlineCompletionItem inline completion items} to be presented in the editor. | ||
*/ |
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 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 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.
Thank you and you're welcome. Of course, sorry I missed it. Done.
5167b24
to
f2d1ba7
Compare
Approved. |
@c-claeys I just noticed that now and it is somehow embarrassing for me: your changes do not compile in the pipeline. The reason is that you already increased the package versions which of course doesn't work since they are not published to npm yet. So you need to undo all the changes were you:
You can always check locally if the pipeline will compile by doing:
We have a different publish pipeline that increases the version numbers which is not public. |
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.
See latest comment about the version numbers.
Head branch was pushed to by a user without write access
d17c7f7
to
59d676d
Compare
I'm fairly certain I addressed the version changes as you requested. That said,
I couldn't regen the metaModel either, even after running |
any ideas on how to implement |
@mroch these two callbacks are proposed in VS Code and in LSP I usually don't adapt proposed API since a library can't opt into using it only extensions. |
…dations (#526) This change adds a concept language server that provides code recommendations from CodeWhisperer. The recommendations are returned through code completions and inline completion. The sample VS Code extension has been updated to support this language server when editing typescript files. This is accomplished by introducing an inline completion provider, which wraps the concept language server. At this time, inline completion is not a part of the language server protocol. VS Code has an extensibility API for inline completion, and a proposal is part of the next protocol version 3.18 (microsoft/language-server-protocol#1673). This proposal is modelled after the extensibility API. A likely implementation of the protocol is proposed in microsoft/vscode-languageserver-node#1190, and portions of this change leverage the shapes defined in that proposal. This way, if or when inline completion is a part of the spec, we could update code to reference `vscode-languageserver` related types with little effort. This concept uses IAM credentials with CodeWhisperer as a way to get up and iterating quickly. In future work we will want to switch over to using bearer tokens, like the AWS Toolkit for VS Code does. The CodeWhisperer service client was produced using the AWS Toolkit for VS Code's client generator (https://github.com/aws/aws-toolkit-vscode/blob/master/scripts/build/generateServiceClient.ts) This language server is being submitted so that we can perform additional experimentation such as: - verifying browser compatibility - exploring the UX around surfacing code recommendations - future bearer token support
…dations (#526) This change adds a concept language server that provides code recommendations from CodeWhisperer. The recommendations are returned through code completions and inline completion. The sample VS Code extension has been updated to support this language server when editing typescript files. This is accomplished by introducing an inline completion provider, which wraps the concept language server. At this time, inline completion is not a part of the language server protocol. VS Code has an extensibility API for inline completion, and a proposal is part of the next protocol version 3.18 (microsoft/language-server-protocol#1673). This proposal is modelled after the extensibility API. A likely implementation of the protocol is proposed in microsoft/vscode-languageserver-node#1190, and portions of this change leverage the shapes defined in that proposal. This way, if or when inline completion is a part of the spec, we could update code to reference `vscode-languageserver` related types with little effort. This concept uses IAM credentials with CodeWhisperer as a way to get up and iterating quickly. In future work we will want to switch over to using bearer tokens, like the AWS Toolkit for VS Code does. The CodeWhisperer service client was produced using the AWS Toolkit for VS Code's client generator (https://github.com/aws/aws-toolkit-vscode/blob/master/scripts/build/generateServiceClient.ts) This language server is being submitted so that we can perform additional experimentation such as: - verifying browser compatibility - exploring the UX around surfacing code recommendations - future bearer token support
This is an initial pass per the discussion in microsoft/language-server-protocol#1529 I primarily modeled it after the existing VSCode API. I'm opening a PR for
language-server-protocol
in parallel.I copied the metaModel.json changes from the other repo, so it looks like some spell/grammar changes were picked up too.