GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Add support for completions inside string literals by mhegazy · Pull Request #8428 · microsoft/TypeScript

Conversation

@mhegazy

@mhegazy Mohamed Hegazy (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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
duanyao and NoelAbrahams reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
Mohamed Hegazy (mhegazy) added 3 commits May 2, 2016 16:29
@mhegazy
Rewrite isInStringLiteral to accomodate for unterminated strings
21587aa
@mhegazy
Refactor signatureHelp to expose helper functions
0a277a1
@mhegazy
Add support for completion in string literals
060f2a8
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-not-required label May 2, 2016
@mhegazy
Remove unused check
7b0d664
DanielRosenwasser
DanielRosenwasser reviewed May 3, 2016
View reviewed changes
Comment thread src/services/services.ts
}
}

function getStringLiteralCompletionEntriesFromCallExpression(argumentInfo: SignatureHelp.ArgumentListInfo) {

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) May 3, 2016

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mhegazy Mohamed Hegazy (mhegazy) May 3, 2016

Copy link
Copy Markdown
Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented May 3, 2016

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
DanielRosenwasser
DanielRosenwasser reviewed May 3, 2016
View reviewed changes
Comment thread src/services/services.ts
return undefined;
}

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

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) May 3, 2016

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mhegazy Mohamed Hegazy (mhegazy) May 3, 2016

Copy link
Copy Markdown
Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Mohamed Hegazy (mhegazy) added 5 commits May 3, 2016 12:18
@mhegazy
Merge branch 'master' into stringLiteralCompletions
4464a41
@mhegazy
Use const instead of let
291ad33
@mhegazy
Fix error
e5a32b7
@mhegazy
Formatting changes
02f30ff
@mhegazy
Use shorthand properties
b9ab4d3
@mhegazy

Mohamed Hegazy (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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mhegazy

Mohamed Hegazy (mhegazy) commented May 3, 2016

Copy link
Copy Markdown
Contributor Author

Yui (@yuit) and Daniel Rosenwasser (@DanielRosenwasser) any more comments?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
yuit
yuit reviewed May 3, 2016
View reviewed changes
Comment thread src/services/services.ts
if (argumentInfo) {
return getStringLiteralCompletionEntriesFromCallExpression(argumentInfo);
}
else if (isElementAccessExpression(node.parent) && node.parent.argumentExpression === node) {

@yuit Yui (yuit) May 3, 2016

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mhegazy Mohamed Hegazy (mhegazy) May 3, 2016

Copy link
Copy Markdown
Contributor Author

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"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@yuit

Yui (yuit) commented May 3, 2016

Copy link
Copy Markdown
Contributor

others than the small comments, look good 🍰.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
yuit
yuit reviewed May 3, 2016
View reviewed changes
Comment thread src/services/services.ts
const entries: CompletionEntry[] = [];

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

@yuit Yui (yuit) May 3, 2016

Copy link
Copy Markdown
Contributor

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mhegazy Mohamed Hegazy (mhegazy) May 3, 2016

Copy link
Copy Markdown
Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mhegazy
Merge branch 'master' into stringLiteralCompletions
f657e3c
@mhegazy Mohamed Hegazy (mhegazy) mentioned this pull request May 16, 2016
Validate indexed properties #8620
Closed
Mohamed Hegazy (mhegazy) added 2 commits June 8, 2016 13:19
@mhegazy
Merge branch 'master' into stringLiteralCompletions
9027da4
@mhegazy
Add comments
cc5dd5b
@mhegazy
Mohamed Hegazy (mhegazy) merged commit 17b5415 into master Jun 8, 2016
@mhegazy
Mohamed Hegazy (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.

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@mhegazy @DanielRosenwasser @yuit @Thaina @vladima @msftclas
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.