Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Add support for completions inside string literals#8428

Merged
mhegazy merged 12 commits into
mastermicrosoft/TypeScript:masterfrom
stringLiteralCompletionsmicrosoft/TypeScript:stringLiteralCompletionsCopy head branch name to clipboard
Jun 8, 2016
Merged

Add support for completions inside string literals#8428
mhegazy merged 12 commits into
mastermicrosoft/TypeScript:masterfrom
stringLiteralCompletionsmicrosoft/TypeScript:stringLiteralCompletionsCopy head branch name to clipboard

Conversation

@mhegazy

@mhegazy mhegazy commented May 2, 2016

Copy link
Copy Markdown
Contributor

Fixes #606

This supports completions in string literals in the following locations:

  1. argument to a call expression whose target parameter has a StringLiteralType

    declare function f(a: "A", b: number): void;
    declare function f(a: "B", b: number): void;
    declare function f(a: "C", b: number): void;
    
    f(" // A | B | C
  2. argument expression of an element access expression

    var o = {
    "#invlaid identifier name": 1
    };
    
    o["| // #invlaid identifier name
  3. a string literal contextually typed by a string literal type

    type Options = "Option 1" | "Option 2" | "Option 3";
    var x: Options = "| // Option 1 | Option 2 | Option 3

This PR does not cover:

  • module name completion
  • changing o. to o[""] if the completion entry is not a valid identifier name

Comment thread src/services/services.ts
}
}

function getStringLiteralCompletionEntriesFromCallExpression(argumentInfo: SignatureHelp.ArgumentListInfo) {

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

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.

@DanielRosenwasser

Copy link
Copy Markdown
Member

Very cool! Looks like this doesn't include the quote character, right?

Comment thread src/services/services.ts
return undefined;
}

const argumentInfo = SignatureHelp.getContainingArgumentInfo(node, position, sourceFile);

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

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.

@mhegazy

mhegazy commented May 3, 2016

Copy link
Copy Markdown
Contributor Author

Very cool! Looks like this doesn't include the quote character, right?

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.

@mhegazy

mhegazy commented May 3, 2016

Copy link
Copy Markdown
Contributor Author

@yuit and @DanielRosenwasser any more comments?

Comment thread src/services/services.ts
if (argumentInfo) {
return getStringLiteralCompletionEntriesFromCallExpression(argumentInfo);
}
else if (isElementAccessExpression(node.parent) && node.parent.argumentExpression === node) {

Copy link
Copy Markdown
Contributor

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"?

Copy link
Copy Markdown
Contributor Author

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"

@yuit

yuit commented May 3, 2016

Copy link
Copy Markdown
Contributor

others than the small comments, look good 🍰.

Comment thread src/services/services.ts
const entries: CompletionEntry[] = [];

if (isSourceFileJavaScript(sourceFile)) {
const uniqueNames = getCompletionEntriesFromSymbols(symbols, entries);

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Contributor Author

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.

@mhegazy mhegazy merged commit 17b5415 into master Jun 8, 2016
@mhegazy mhegazy deleted the stringLiteralCompletions branch June 8, 2016 21:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.