Filter out existing cases from string completions #52633
Conversation
typescript-bot
added
the
For Uncommitted Bug
src/services/stringCompletions.ts
Outdated
| case StringLiteralCompletionKind.Types: | ||
| return find(completion.types, t => t.value === name) ? createCompletionDetails(name, ScriptElementKindModifier.none, ScriptElementKind.typeElement, [textPart(name)]) : undefined; | ||
| case StringLiteralCompletionKind.Cases: | ||
| return; |
There was a problem hiding this comment.
I wasn't sure what kind of details should be returned here.
There was a problem hiding this comment.
I think you should do something like the above case for StringLiteralCompletionKind.Types, except I don't think the kind should be ScriptElementKind.typeElement, it should be ScriptElementKind.string instead. Not related to your PR: I don't know why the call above is using ScriptElementKind.typeElement, it looks weird to me that the kind in the CompletionEntry and CompletionEntryDetails return two different things for StringLiteralCompletionKind.Types case, do you know why that is @andrewbranch?
There was a problem hiding this comment.
I came to the conclusion - that perhaps I don't need a completely separate type and I can just use .Types. At first, I wasn't sure what .Types is supposed to resolve so I was afraid of reusing it. WDYT?
There was a problem hiding this comment.
I don’t know the answer to @gabritto’s question, but I do think you can just use .Types for this.
There was a problem hiding this comment.
I think you can reuse it and fix the CompletionEntryDetails.kind weirdness. With any luck, if this turns out to be wrong for some reason, a test will break 😅
There was a problem hiding this comment.
AFAIK that property only controls the icon displayed in editors, and the editors map our categories onto their own, so there’s not really a right or wrong answer.
| hasValue(value: string | number | PseudoBigInt): boolean; | ||
| } | ||
|
|
||
| function newCaseClauseTracker(checker: TypeChecker, clauses: readonly (CaseClause | DefaultClause)[]): CaseClauseTracker { |
There was a problem hiding this comment.
I think it makes sense to move this out to a shared space so both completions and stringCompletions can use it - but if you prefer some other solution, let me know.
typescript-bot
added
For Backlog Bug
| //// case /*2*/ | ||
| //// } | ||
| //// declare const f2: 'foo' | 'bar' | 'baz'; | ||
| //// switch (f) { |
There was a problem hiding this comment.
I think you meant to switch (f2) here instead of switch (f).
There was a problem hiding this comment.
yes, definitely - this was a late copy-paste from a separate file in which I was originally testing it 😅 gonna amend that soon
src/services/stringCompletions.ts
Outdated
| case StringLiteralCompletionKind.Types: | ||
| return find(completion.types, t => t.value === name) ? createCompletionDetails(name, ScriptElementKindModifier.none, ScriptElementKind.typeElement, [textPart(name)]) : undefined; | ||
| case StringLiteralCompletionKind.Cases: | ||
| return; |
There was a problem hiding this comment.
I think you should do something like the above case for StringLiteralCompletionKind.Types, except I don't think the kind should be ScriptElementKind.typeElement, it should be ScriptElementKind.string instead. Not related to your PR: I don't know why the call above is using ScriptElementKind.typeElement, it looks weird to me that the kind in the CompletionEntry and CompletionEntryDetails return two different things for StringLiteralCompletionKind.Types case, do you know why that is @andrewbranch?
|
@Andarist FYI you may want to recopy the settings from |
Andarist
deleted the
fix/filter-out-existing-cases-from-string-completions
branch
5 participants
fixes #52614
cc @gabritto @andrewbranch