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.

Sign up for GitHub

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

Jump to bottom

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
+111 −1
Open

Add support for completionList.applyKind to determine how values from completionList.itemDefaults and completion are combined. #2018

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7ad8da8
Add support for `completionList.applyKind` to determine how values fr…
DanTup Sep 11, 2024
ab78abf
Update completion.md
DanTup Sep 11, 2024
9886c0e
Simplify by removing distinction between null/undefined
DanTup Sep 11, 2024
832f719
Use a single client capability + add ApplyKind enum/type
DanTup Sep 12, 2024
f094493
Make this clearer
DanTup Sep 12, 2024
21ff7ad
Minor fixes
DanTup Sep 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 83 additions & 1 deletion _specifications/lsp/3.18/language/completion.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,18 @@ export interface CompletionClientCapabilities {
* @since 3.17.0
*/
itemDefaults?: string[];

/**
* Specifies which fields of `CompletionList.applyKind` the client
* supports. If omitted, no properties are supported and all fields
* in a completion item will replace the defaults.
*
* Clients may only specify fields that have merge rules defined in the
* LSP spec.
*
* @since 3.18.0
*/
applyKinds?: string[];
Copy link
Contributor

@rchl rchl Sep 11, 2024 •
edited
Loading

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?

}
}
```
Expand Down Expand Up @@ -338,7 +350,8 @@ export interface CompletionList {
* be used if a completion item itself doesn't specify the value.
*
* If a completion list specifies a default value and a completion item
* also specifies a corresponding value, the one from the item is used.
* also specifies a corresponding value, the rules for combining these are
* defined by `applyKinds`, defaulting to "replace".
*
* Servers are only allowed to return default values if the client
* signals support for this via the `completionList.itemDefaults`
Expand Down Expand Up @@ -386,6 +399,75 @@ export interface CompletionList {
data?: LSPAny;
}

/**
* Specifies how fields from a completion item should be combined with those
* from `completionList.itemDefaults`.
*
* In unspecified, all fields will be treated as "replace".
*
* If a field's value is "replace", the value from a completion item will
* always be used instead of the value from `completionItem.itemDefaults`.
*
* If a field's value is "merge", the values will be merged using the rules
* defined against each field below.
*
* Servers are only allowed to return `applyKind` if the client
* signals support for this via the `completionList.applyKinds`
* capability.
*
* @since 3.18.0
*/
applyKind?: {
/**
* Specifies whether commitCharacters on a completion will replace or be
* merged with those in `completionList.itemDefaults.commitCharacters`.
*
* If "replace", the commit characters from the completion item will
* always be used unless not provided, in which case those from
* `completionList.itemDefaults.commitCharacters` will be used. An empty
* list can be used if a completion item does not have any commit
* characters and also should not use those from
* `completionList.itemDefaults.commitCharacters`.
*
* If "merge" the commitCharacters for the completion will be the union
* of all values in both `completionList.itemDefaults.commitCharacters`
* and the completion's own `commitCharacters`. An empty list indicates
* the completion adds no additional commit characters and a value of
* `null` indicates the defaults should not be used.
*
* @since 3.18.0
*/
commitCharacters?: "replace" | "merge";

/**
* Specifies whether data on a completion will replace or
* be merged with data from `completionList.itemDefaults.data`.
*
* If "replace", the data from the completion item will be used if
* provided, otherwise `completionList.itemDefaults.data` will be used.
* An empty object can be used if a completion item does not have any
* data but also should not use the value from
* `completionList.itemDefaults.data`.
*
* If "merge", a shallow merge will be performed between
* `completionList.itemDefaults.data` and the completion's own data
* using the following rules:
*
* - If a completion's data is not specified, the data from
* `completionList.itemDefaults.data` will be used.
* - If a completion's data is null, the resulting data field will be
* null (defaults are not used).
DanTup marked this conversation as resolved.
Show resolved Hide resolved
* - If a field is specified in `completion.data` it will be used as-is.
* - If a field is `null` in `completion.data`, it will remain `null`
* (default is not used).
DanTup marked this conversation as resolved.
Show resolved Hide resolved
* - If a field is unspecified in `completion.data`, the same field from
* `completionList.itemDefaults.data` will be used.
*
* @since 3.18.0
*/
data?: "replace" | "merge";
}

/**
* The completion items.
*/
Expand Down
1 change: 1 addition & 0 deletions _specifications/lsp/3.18/specification.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -737,6 +737,7 @@ Since 3.17 there is a meta model describing the LSP protocol:
* Support for snippets in text document edits.
* Support for debug message kind.
* Client capability to enumerate properties that can be resolved for code lenses.
* Added support for `completionList.applyKind` to determine how values from `completionList.itemDefaults` and `completion` are combined.


#### <a href="#version_3_17_0" name="version_3_17_0" class="anchor">3.17.0 (05/10/2022)</a>
Expand Down