-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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 completions inside string literals #8428
Conversation
} | ||
} | ||
|
||
function getStringLiteralCompletionEntriesFromCallExpression(argumentInfo: SignatureHelp.ArgumentListInfo) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can eventually generalize this logic to other types as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i suppose if we do pre-selecting of completion entries based on contextual type.
Very cool! Looks like this doesn't include the quote character, right? |
return undefined; | ||
} | ||
|
||
const argumentInfo = SignatureHelp.getContainingArgumentInfo(node, position, sourceFile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a slightly weird sort of dependency. Ideally there would be an API on the type checker like getPossibleContextualTypes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not contextual type per se. but sure. If there is another use case for such API we can push it into the checker, but this is the only one, i would keep it at the use site.
yes. i am assuming here that automatic brace completion would get that done for you, or you have them written already. we should add replaceText, and replaceRange on CompletionEntry, to allow that. there are some issues with Roslyn API currently that makes this hard, but i believe should be better in next version. |
@yuit and @DanielRosenwasser any more comments? |
if (argumentInfo) { | ||
return getStringLiteralCompletionEntriesFromCallExpression(argumentInfo); | ||
} | ||
else if (isElementAccessExpression(node.parent) && node.parent.argumentExpression === node) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you have to check this "node.parent.argumentExpression === node"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"expression"["index"]
want to make sure we are on "index"
others than the small comments, look good 🍰. |
const entries: CompletionEntry[] = []; | ||
|
||
if (isSourceFileJavaScript(sourceFile)) { | ||
const uniqueNames = getCompletionEntriesFromSymbols(symbols, entries); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you start perform checking for characters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure really. this is how it was before adding the argument. let me look more into it.
Are there any jsdoc comment syntax I could made to use this feature in javascript with salsa? |
Fixes #606
This supports completions in string literals in the following locations:
argument to a call expression whose target parameter has a StringLiteralType
argument expression of an element access expression
a string literal contextually typed by a string literal type
This PR does not cover:
o.
too[""]
if the completion entry is not a valid identifier name