-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Add optionalReplacementSpan to completions response #40347
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
Changes from all commits
5d2b5d9
ee3d9ae
0c08e69
45363a8
14cb4ff
dd7dbe0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -5964,6 +5964,12 @@ declare namespace ts { | |||
/** Not true for all global completions. This will be true if the enclosing scope matches a few syntax kinds. See `isSnippetScope`. */ | ||||
isGlobalCompletion: boolean; | ||||
isMemberCompletion: boolean; | ||||
/** | ||||
* In the absence of `CompletionEntry["replacementSpan"], the editor may choose whether to use | ||||
* this span or its default one. If `CompletionEntry["replacementSpan"]` is defined, that span | ||||
* must be used to commit that completion entry. | ||||
*/ | ||||
optionalReplacementSpan?: TextSpan; | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @andrewbranch is this correct? The examples above in the unittests show TextSpan being used. But this is pointing to here, is it using the correct interface? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://github.com/microsoft/TypeScript/blob/main/src/testRunner/unittests/tsserver/metadataInResponse.ts#L83-L86 its still set to that interface also There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, internally it should be a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
im having a nightmare with a typescript server plugin not having correct completion data, mainly range data, see here microsoft/vscode#134328 I thought maybe it needs to include This new field but TextSpan didn’t match with the examples here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I though the culprit was here https://github.com/microsoft/typescript-styled-plugin/blob/main/src/_language-service.ts#L121-L138 because Looking at the types for CompletionEntry and CompletionEntryDetails I don't see where the range would be passed through back to VSCode There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here’s where it gets converted on its way to VS Code TypeScript/src/server/session.ts Line 1873 in 9766757
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks Andrew. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @andrewbranch in case you were curious, this was the fix, took me days: microsoft/typescript-styled-plugin#156 |
||||
/** | ||||
* true when the current location also allows for a new identifier | ||||
*/ | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
/// <reference path="fourslash.ts" /> | ||
|
||
// @lib: dom | ||
|
||
//// console.[|cl/*0*/ockwork|]; | ||
//// type T = Array["[|toS/*1*/paghetti|]"]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure why this has to be optionalReplacementSpan and not just replacementSpan if editor supports it (there is already preference for that no) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My understanding is there is not a preference that allows users to opt out of using That said, it does seem like using the new There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But i dont see this being configured by preference? Even if this is configured by preference, why cant it be returned as part of replacementSpan since replacement span if present is always meant to override this one? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe it’s supposed to be an editor preference, which could potentially be configured across multiple languages. If TS Server adopted this preference and VS Code sent it to us, that would be an option. That’s not something that was discussed in the issue; I personally don’t have strong feelings one way or another. I’ll let @mjbvz weigh in. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. VS code lets users configure how suggestions are inserted using the However users can always toggle to the opposite mode of the current one by holding down
I think the current proposal covers our needs but let me know if that doesn't sound correct based on my description There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it. Now understand why you always want this property to be populated and separate. Thanks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the clarification 👍 |
||
|
||
test.ranges().forEach((range, marker) => { | ||
verify.completions({ | ||
marker: `${marker}`, | ||
optionalReplacementSpan: range, | ||
}); | ||
}); |
Uh oh!
There was an error while loading. Please reload this page.