Add API option to `getCompletionsAtPosition` to expose completion `symbol` information #52560

zardoy · 2023-02-02T04:25:22Z

Fixes #51936

#51936 (comment)

Only 2 tests fail: The baseline file api/typescript.d.ts has changed.

@andrewbranch Let me know if I'm going here into right direction.

andrewbranch

I think I’m ok with this if the implementation can be restructured to conditionally assign rather than always assign and conditionally delete. Beyond that, the API might need a little bikeshedding. Some things to consider:

The name of the option
Once we settle on the API shape, we will want to add JSDoc explaining the functionality and caveats (e.g. the symbol may retain a whole TypeChecker in it, so don’t store it longer than you need it!)
Is it worth it to make a getCompletionsAtPosition overload for when the option is true that changes the return type?
Should the symbol go right on CompletionEntry, or should it be wrapped in an apiData property or something?

@jakebailey @gabritto do you have any input or overall concerns?

src/services/types.ts

src/services/completions.ts

gabritto · 2023-02-03T00:57:13Z

I don't think I have any concerns, this looks fine besides the comments you already left. @andrewbranch I do have a question though: if we make the symbol part of the API, what would that imply for us regarding future, possibly breaking changes in completions? For example, until recently I think (but I might be wrong) we only had one completion entry per symbol, but that changed with object method completions (although that feature can be disabled). Would we have to be more mindful of that kind of change going forward?

zardoy · 2023-02-03T09:12:44Z

src/services/stringCompletions.ts

+                /*symbolToSortTextMap*/ undefined,
+                /*isJsxIdentifierExpected*/ undefined,
+                /*isRightOfOpenTag*/ undefined,
+                includeApiData


In first version this case wasn't handled, now fixed to be consistent

zardoy · 2023-02-03T09:42:06Z

Should the symbol go right on CompletionEntry, or should it be wrapped in an apiData property or something?

From my perpective it is easiest solution for us and most convenient for consumer

Also feel free to edit jsdoc

typescript-bot · 2023-02-03T09:42:20Z

The TypeScript team hasn't accepted the linked issue #51936. If you can get it accepted, this PR will have a better chance of being reviewed.

src/services/completions.ts

Co-authored-by: Andrew Branch <[email protected]>

andrewbranch · 2023-02-03T17:53:11Z

For example, until recently I think (but I might be wrong) we only had one completion entry per symbol, but that changed with object method completions (although that feature can be disabled). Would we have to be more mindful of that kind of change going forward?

That’s not an issue for this particular change, since still every completion has at most one symbol. The only way this property could break a future API change is if individual completions started being derived from multiple symbols 🤔.

Thinking about it this morning, though, I’m wondering if the change needs to be so generic. What additional data might someone need on a completion item in the future? Maybe we should just call the option includeSymbol, and put symbol as an optional property on CompletionEntry. The extra types and overload really don’t help that much, because symbol is always optional at best.

zardoy · 2023-02-03T18:10:46Z

Maybe we should just call the option includeSymbol, and put symbol as an optional property on CompletionEntry.

tbh I more like general name, as idea can be extended without adding new properties. Like what if also type to string completions or container of completions will also be added in future (though I cant imagine a usecase for them for now)? Anyway, its just a naming for current implementation so let me know what you think guys.

The extra types and overload really don’t help that much, because symbol is always optional at best.

It will change already existing calls so plugin devs might start wondering what is new symbol property is added and why its never defined. On the other hand jsdoc can be added to clarify it, so this is totally fine to me. Don't mind of any change here.

zardoy · 2023-02-15T12:38:35Z

To summarize requested changes here:

Replace all includeApiData to includeSymbol
Remove overload, CompletionEntryWithApiData and add symbol to just CompletionEntry

@andrewbranch Is that right and anything else to consider?

I don't really care of typings and so on, I'm only interested in adding a way to use symbol :)

P.S. feel free to make quick edits directly!

src/services/types.ts

andrewbranch · 2023-02-16T00:58:03Z

Replace all includeApiData to includeSymbol

Remove overload, CompletionEntryWithApiData and add symbol to just CompletionEntry

Discussed this in #52790 and yes, along with my suggested JSDoc change, if you do these two things it should be good to go 👍

Co-authored-by: Andrew Branch <[email protected]>

…d someone might not know about added option

zardoy · 2023-02-16T08:20:03Z

src/services/types.ts

+     * Included for non-string completions only when `includeSymbol: true` option is passed to `getCompletionsAtPosition`.
+     * @example Get declaration of completion: `symbol.valueDeclaration`
+     */
+    symbol?: Symbol


@andrewbranch All done, but decided to also add jsdoc on symbol property itself (since its always visible to user even if option is off). Anyway, don't see anything bad in extra documentation 😄

andrewbranch

Looks good, just run the 'Public API' test and accept the baselines to make the tests green

zardoy added 2 commits February 2, 2023 06:56

is that simple?

f9a00e2

poc

96898d4

typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Feb 2, 2023

fix eslint

99c7458

zardoy changed the title ~~is that simple?~~ Add and option to getCompletionsAtPosition to expose completion symbol information Feb 2, 2023

andrewbranch requested changes Feb 2, 2023

View reviewed changes

src/services/types.ts Outdated Show resolved Hide resolved

src/services/completions.ts Outdated Show resolved Hide resolved

zardoy added 2 commits February 3, 2023 11:45

rename public option + add jsdoc, add ls overload

92dcef7

add symbol conditionally pass includeApiData to createCompletionEntry

941c080

zardoy commented Feb 3, 2023

View reviewed changes

zardoy marked this pull request as ready for review February 3, 2023 09:42

andrewbranch reviewed Feb 3, 2023

View reviewed changes

src/services/completions.ts Outdated Show resolved Hide resolved

up longer syntax

ae2a141

Co-authored-by: Andrew Branch <[email protected]>

zardoy changed the title ~~Add and option to getCompletionsAtPosition to expose completion symbol information~~ Add API option to getCompletionsAtPosition to expose completion symbol information Feb 11, 2023

andrewbranch mentioned this pull request Feb 15, 2023

Design Meeting Notes, 2/15/2023 #52790

Closed

andrewbranch reviewed Feb 16, 2023

View reviewed changes

src/services/types.ts Outdated Show resolved Hide resolved

zardoy and others added 4 commits February 16, 2023 11:02

Update src/services/types.ts

6818e41

Co-authored-by: Andrew Branch <[email protected]>

remove added interface, remove overload

601a610

rename all includeApiData -> includeSymbol

2259d1a

add jsdoc describing symbol Why? Its always undefined by default an…

5073d8e

…d someone might not know about added option

zardoy commented Feb 16, 2023

View reviewed changes

andrewbranch approved these changes Feb 16, 2023

View reviewed changes

pnpm hereby baseline-accept

d886f8c

zardoy requested a review from andrewbranch February 16, 2023 18:01

andrewbranch merged commit 2f43308 into microsoft:main Feb 16, 2023

zardoy deleted the expose-completion-symbol branch February 17, 2023 13:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API option to `getCompletionsAtPosition` to expose completion `symbol` information #52560

Add API option to `getCompletionsAtPosition` to expose completion `symbol` information #52560

zardoy commented Feb 2, 2023

andrewbranch left a comment •

edited

Loading

gabritto commented Feb 3, 2023

zardoy Feb 3, 2023

zardoy commented Feb 3, 2023 •

edited

Loading

typescript-bot commented Feb 3, 2023

andrewbranch commented Feb 3, 2023

zardoy commented Feb 3, 2023 •

edited

Loading

zardoy commented Feb 15, 2023 •

edited

Loading

andrewbranch commented Feb 16, 2023

zardoy Feb 16, 2023

andrewbranch left a comment

Add API option to getCompletionsAtPosition to expose completion symbol information #52560

Add API option to getCompletionsAtPosition to expose completion symbol information #52560

Conversation

zardoy commented Feb 2, 2023

andrewbranch left a comment • edited Loading

Choose a reason for hiding this comment

gabritto commented Feb 3, 2023

zardoy Feb 3, 2023

Choose a reason for hiding this comment

zardoy commented Feb 3, 2023 • edited Loading

typescript-bot commented Feb 3, 2023

andrewbranch commented Feb 3, 2023

zardoy commented Feb 3, 2023 • edited Loading

zardoy commented Feb 15, 2023 • edited Loading

andrewbranch commented Feb 16, 2023

zardoy Feb 16, 2023

Choose a reason for hiding this comment

andrewbranch left a comment

Choose a reason for hiding this comment

Add API option to `getCompletionsAtPosition` to expose completion `symbol` information #52560

Add API option to `getCompletionsAtPosition` to expose completion `symbol` information #52560

andrewbranch left a comment •

edited

Loading

zardoy commented Feb 3, 2023 •

edited

Loading

zardoy commented Feb 3, 2023 •

edited

Loading

zardoy commented Feb 15, 2023 •

edited

Loading