add supports of completion label list by Kingwl · Pull Request #20362 · microsoft/TypeScript

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

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

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


i would combine these two loops.

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

        return false;
    }

+    export function isBreakOrContinue(kind: SyntaxKind) {


we already have isBreakOrContinueStatement

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

+
+    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

    }

+    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


+    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

    }

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


Unnecessary type annotation

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

        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

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


Use .text to avoid the cast.

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

        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


+    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

+    }
+
+    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.

Conversation

Kingwl commented Nov 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhegazy commented Dec 2, 2017

Uh oh!

Kingwl commented Dec 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kingwl commented Dec 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kingwl commented Dec 6, 2017

Uh oh!

ghost commented Dec 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants