Add support for completionList.applyKind to determine how values from completionList.itemDefaults and completion are combined.#2018
Conversation
…om `completionList.itemDefaults` and `completion` are combined. Fixes microsoft#1802
| * | ||
| * @since 3.18.0 | ||
| */ | ||
| applyKinds?: string[]; |
There was a problem hiding this comment.
This seems to rely on itemDefaults capability being supported. Would it make it more obvious if it would be called something like itemDefaultsApplyKinds?
And another discussion could be had about the "applyKinds" naming. It doesn't feel very descriptive to me (perhaps subjective). Also it refers to "kind" which is a concept that is already present in completion items that is not related to "kinds" here. Perhaps a better one would be `itemDefaultsMergeStrategy" (although it might be weird to call it "merge strategy" when the strategy can either replace or merge.
There was a problem hiding this comment.
I don't feel strongly about the name, though I think like apply more than merge since as you say, merge is one option. Maybe something like "combine" is a bit clearer? I do agree kind maybe could be clearer (strategy? method?)
I had a scan through the spec to see if there's anything similar we could name it the same as, but I couldn't find anything that feels the same.
There was a problem hiding this comment.
I would name it applyKindSupport: boolean. We should also add a ApplyKind definition since the spec usually doesn't use inline string or types something like
export namespace ApplyKind {
export const Replace: 'replace' = 'replace;
export const Merge: 'merge' = 'merge';
}
export type ApplyKind = ApplyKind.Replace | ApplyKind.Merge;
I do think one boolean is enough due to: if a client supports the new filed (e.g. via itemDefaults) and it opts into applyKindSupport it MUST then support the new filed in the applyKind literal. I think that is fair.
There was a problem hiding this comment.
And you are fine with using the kind wording even though completion items already have a concept of kind that is something completely different?
There was a problem hiding this comment.
@dbaeumer I've pushed changes to this affect - PTAL! I did wonder if we should use ApplyKind or Completion(Item?)ApplyKind.. I don't know if we might support replace/merge elsewhere in future and if so we'd want to use the same enum or not?
And you are fine with using the kind wording even though completion items already have a concept of kind
There are a lot of kinds in the spec so I don't think reusing kind is bad (it's CompletionItemKind vs ApplyKind), although I'll note we also have InsertTextMode which is a "mode", so that's possibly another option?
|
I pushed some tweaks to remove the different handling for |
dbaeumer
left a comment
There was a problem hiding this comment.
Looks good to me. I will merge when we have an implementation
- commitCharacters isn't allowed to be null so no point mentioning this - Fix missing quote - Fix error in ApplyKind type (either needs "typeof" or to use literals, and other things use literals)
…d per-item commitCharacters/data are combined Implements the changes in the LSP spec PR at microsoft/language-server-protocol#2018. (Also see microsoft/language-server-protocol#1802)
|
Great! I've opened a PR at microsoft/vscode-languageserver-node#1558 :-) |
|
I've pushed 37f21df and 9081d2f with tweaks discussed at microsoft/vscode-languageserver-node#1558 (comment) |
…d per-item commitCharacters/data are combined (#1558) * Add support for CompletionList "applyKind" to control how defaults and per-item commitCharacters/data are combined Implements the changes in the LSP spec PR at microsoft/language-server-protocol#2018. (Also see microsoft/language-server-protocol#1802) * Update meta model * Add non-null falsy test * Change ApplyKind to ints * Tweaks + typos --------- Co-authored-by: Dirk Bäumer <dirkb@microsoft.com>
The goal here is to significantly reduce payload sizes by allowing the server to avoid duplicating values across all
completion.commitCharactersandcompletion.datathat are the same for all (or most) items.Only the fields specified in the spec support
mergebecause the exact merge semantics must be specified. A client capability indicates which fields a given client supports merging.@dbaeumer I implemented it this way because I worry that a single boolean may cause issues in future if new fields are added that we might want to support merge for, but I am happy to change if you don't like this.
Fixes #1802