add supports of completion label list #20362

Kingwl · 2017-11-30T08:50:30Z

mhegazy · 2017-12-02T01:30:14Z

src/services/completions.ts

+            current = current.parent;
+        }
+
+        for (const stmt of statements) {


i would combine these two loops.

mhegazy · 2017-12-02T01:31:28Z

src/services/utilities.ts

@@ -857,6 +857,15 @@ namespace ts {
        return false;
    }

+    export function isBreakOrContinue(kind: SyntaxKind) {


we already have isBreakOrContinueStatement

mhegazy · 2017-12-02T01:34:19Z

src/services/utilities.ts

+
+    export function isInBreakOrContinue(sourceFile: SourceFile, position: number): boolean {
+        const contextToken = findPrecedingToken(position, sourceFile);
+        return contextToken && (isBreakOrContinue(contextToken.kind) || contextToken.parent && isIdentifier(contextToken) && isBreakOrContinueStatement(contextToken.parent));


we already do this in getCompletionData, consider moving the check for isLabeledCompletion to that function.

mhegazy · 2017-12-02T01:36:02Z

@Andy-MS can you please review.

Kingwl · 2017-12-04T02:30:18Z

ping @mhegazy
thanks for you review
I did the following after your review

merged two loops
remove the isBreakOrContrinue
simplify the type of judgment

But I still prefer to keep completion Label alone instead of putting it in GetCompletionData.
the GetCompletionData return the interface CompletionData with the symbols field.
but the label filed of labelStmt is Identity
so have i miss something?

ghost · 2017-12-04T16:18:39Z

src/services/completions.ts

@@ -223,6 +227,21 @@ namespace ts.Completions {
        return uniques;
    }

+    function getLabelCompletionAtPosition(sourceFile: SourceFile, position: number): CompletionInfo | undefined {
+        const contextToken: Node = findPrecedingToken(position, sourceFile);


Just pass in the token.

ghost · 2017-12-04T16:18:55Z

src/services/completions.ts

@@ -223,6 +227,21 @@ namespace ts.Completions {
        return uniques;
    }

+    function getLabelCompletionAtPosition(sourceFile: SourceFile, position: number): CompletionInfo | undefined {
+        const contextToken: Node = findPrecedingToken(position, sourceFile);
+        if (!contextToken || !contextToken.parent ||


You just tested for this in isInBreakOrContinue, right?

ghost · 2017-12-04T16:20:24Z

src/services/completions.ts

@@ -358,6 +377,28 @@ namespace ts.Completions {
        return undefined;
    }

+    function addLabelStatementCompletions(node: Node, entries: CompletionEntry[], uniques = createMap<true>()): void {
+        let current: Node = node;


Unnecessary type annotation

ghost · 2017-12-04T16:21:06Z

src/services/completions.ts

@@ -358,6 +377,28 @@ namespace ts.Completions {
        return undefined;
    }

+    function addLabelStatementCompletions(node: Node, entries: CompletionEntry[], uniques = createMap<true>()): void {


This really only depends on the node. You can remove the uniques parameter and just use const uniques = createMap<true>, and remove the entries parameter and just return an array.

ghost · 2017-12-04T16:21:56Z

src/services/completions.ts

+                break;
+            }
+            if (isLabeledStatement(current)) {
+                const name = current.label.escapedText as string;


Use .text to avoid the cast.

ghost · 2017-12-04T16:24:33Z

src/services/utilities.ts

@@ -857,6 +857,13 @@ namespace ts {
        return false;
    }

+    export function isInBreakOrContinue(sourceFile: SourceFile, position: number): boolean {


I would just move this to completions.ts since it's unlikely to be needed outside of a completion context.

ghost · 2017-12-04T16:27:42Z

src/services/utilities.ts

@@ -857,6 +857,13 @@ namespace ts {
        return false;
    }

+    export function isInBreakOrContinue(sourceFile: SourceFile, position: number): boolean {
+        const contextToken = findPrecedingToken(position, sourceFile);
+        return contextToken && contextToken.parent &&


If the kind matches, the parent will exist (since it's not a SourceFile), so no need to test for && contextToken.parent.

Kingwl · 2017-12-05T02:13:02Z

ping @Andy-MS
thanks for your review
i remove the Unnecessary parameter, type annotation, cast and type test
but i'm not sure what means Just pass in the token.

ghost · 2017-12-05T17:30:38Z

src/services/completions.ts

+    }
+
+    function getLabelCompletionAtPosition(sourceFile: SourceFile, position: number): CompletionInfo | undefined {
+        const contextToken = findPrecedingToken(position, sourceFile);


This was just computed in isBreakOrContinue, move this outside so it's only computed once.

Kingwl · 2017-12-06T01:45:18Z

ping @Andy-MS updated

ghost · 2017-12-06T15:01:29Z

Thanks!

Kingwl changed the title ~~add supports of completion label list~~ WIP: add supports of completion label list Nov 30, 2017

Kingwl force-pushed the label-completion branch from 10c3a7c to 117aa03 Compare November 30, 2017 09:13

Kingwl changed the title ~~WIP: add supports of completion label list~~ add supports of completion label list Nov 30, 2017

mhegazy reviewed Dec 2, 2017

View reviewed changes

mhegazy requested a review from a user December 2, 2017 01:35

Kingwl force-pushed the label-completion branch 2 times, most recently from ee9998e to c424e67 Compare December 4, 2017 02:21

Kingwl force-pushed the label-completion branch from c424e67 to 429ba67 Compare December 4, 2017 02:46

ghost reviewed Dec 4, 2017

View reviewed changes

mhegazy requested a review from a user December 4, 2017 19:05

Kingwl force-pushed the label-completion branch from 429ba67 to f14448b Compare December 5, 2017 02:09

Kingwl force-pushed the label-completion branch from f14448b to b4ac6f5 Compare December 5, 2017 03:21

ghost reviewed Dec 5, 2017

View reviewed changes

add supports of completion label list

2d9e9a2

Kingwl force-pushed the label-completion branch from b4ac6f5 to 2d9e9a2 Compare December 6, 2017 01:45

ghost merged commit ae25d09 into microsoft:master Dec 6, 2017

ghost mentioned this pull request Dec 6, 2017

Minor cleanup for label completions #20502

Merged

Kingwl deleted the label-completion branch December 6, 2017 15:30

microsoft locked and limited conversation to collaborators Jun 14, 2018

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add supports of completion label list #20362

add supports of completion label list #20362

Kingwl commented Nov 30, 2017

mhegazy Dec 2, 2017

mhegazy Dec 2, 2017

mhegazy Dec 2, 2017

mhegazy commented Dec 2, 2017

Kingwl commented Dec 4, 2017

ghost Dec 4, 2017

ghost Dec 4, 2017

ghost Dec 4, 2017

ghost Dec 4, 2017

ghost Dec 4, 2017

ghost Dec 4, 2017

ghost Dec 4, 2017

Kingwl commented Dec 5, 2017

ghost Dec 5, 2017

Kingwl commented Dec 6, 2017

ghost commented Dec 6, 2017

add supports of completion label list #20362

add supports of completion label list #20362

Conversation

Kingwl commented Nov 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhegazy commented Dec 2, 2017

Kingwl commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kingwl commented Dec 5, 2017

Choose a reason for hiding this comment

Kingwl commented Dec 6, 2017

ghost commented Dec 6, 2017