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 support for completionList.applyKind to determine how values from completionList.itemDefaults and completion are combined. #2018

Open
wants to merge 6 commits into
base: gh-pages
Choose a base branch
from

Conversation

DanTup
Copy link
Contributor

@DanTup DanTup commented Sep 11, 2024

The goal here is to significantly reduce payload sizes by allowing the server to avoid duplicating values across all completion.commitCharacters and completion.data that are the same for all (or most) items.

Only the fields specified in the spec support merge because 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

…om `completionList.itemDefaults` and `completion` are combined.

Fixes microsoft#1802
*
* @since 3.18.0
*/
applyKinds?: string[];
Copy link
Contributor

@rchl rchl Sep 11, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

And you are fine with using the kind wording even though completion items already have a concept of kind that is something completely different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

@rchl

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?

@DanTup
Copy link
Contributor Author

DanTup commented Sep 11, 2024

I pushed some tweaks to remove the different handling for null values (instead, indicating that null values will be treated the same as undefined). This does make it a little more difficult if you want to set some defaults and turn them off for some completion items, but I think it might not be worth the additional complexity for this.

Copy link
Member

@dbaeumer dbaeumer left a comment

Choose a reason for hiding this comment

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

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)
@DanTup
Copy link
Contributor Author

DanTup commented Sep 18, 2024

Great! I've opened a PR at microsoft/vscode-languageserver-node#1558 :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants