Support a 'recommended' completion entry #20020

ghost · 2017-11-14T22:24:06Z

Fixes #19855
This adds an isRecommended boolean to one (or no) completion entry, when that entry is for a enum or (non-abstract) class and the context indicates that that's the only allowed type.

armanio123 · 2017-11-16T23:19:04Z

src/services/completions.ts

+            !!(localSymbol.flags & SymbolFlags.ExportValue) && checker.getExportSymbolOfSymbol(localSymbol) === recommendedCompletion;
+    }
+
+    function trueOrUndef(b: boolean): true | undefined {


Avoid abbreviations.

armanio123 · 2017-11-16T23:23:26Z

src/services/completions.ts

+
+    function getFirstSymbolInChain(symbol: Symbol, enclosingDeclaration: Node, checker: TypeChecker): Symbol | undefined {
+        const chain = checker.getAccessibleSymbolChain(symbol, enclosingDeclaration, /*meaning*/ SymbolFlags.All, /*useOnlyExternalAliasing*/ false);
+        return chain


Nested shorthands are hard to read. Consider if else, or switch statement.

ghost · 2017-11-17T23:03:52Z

Should probably get review from @mjbvz

mjbvz · 2017-11-17T23:20:11Z

Just to confirm, the expected UX here would be that we surface the recommended completion first? Should that also happen if the user has already started typing something? Would sort order be enough here then?

mhegazy · 2017-11-20T17:34:06Z

src/compiler/checker.ts

@@ -13876,6 +13859,12 @@ namespace ts {
                case SyntaxKind.AmpersandAmpersandToken:
                case SyntaxKind.CommaToken:
                    return node === right ? getContextualType(binaryExpression) : undefined;
+                case SyntaxKind.EqualsEqualsEqualsToken:


this changes what contextual type mean in the language. we should talk about this first

mhegazy · 2017-11-20T17:34:30Z

src/compiler/checker.ts

                case SyntaxKind.NewExpression:
-                    return getContextualTypeForArgument(<CallExpression>parent, node);
+                    if (node.kind === SyntaxKind.NewKeyword) { // for completions after `new `


this one too

mhegazy · 2017-11-20T17:37:20Z

src/services/types.ts

        sortText: string;
-        /**


why is this comment removed

mhegazy · 2017-11-20T17:38:44Z

src/services/types.ts

        replacementSpan?: TextSpan;
        hasAction?: true;
        source?: string;
+        isRecommended?: true;


i would rather we put this on the CompletionInfo instead. i understand for the server we have to put it on the entry.. but for VS for example, it makes more sense to have it on the info..
Please check with @amcasey about this too.

I'm not intimately familiar with these types. The info is the wrapper object containing a list of entries? Assuming that's the case, having it on the entry is the most convenient place for VS.

It's defined at interface CompletionInfo in services/types.ts

Then, yes, the entry would be preferable to the info. I've created a PR with the VS change if you'd like to test it.

mhegazy · 2017-11-20T17:48:07Z

Just to confirm, the expected UX here would be that we surface the recommended completion first? Should that also happen if the user has already started typing something? Would sort order be enough here then?

Visual Studio will show the recommended completion as selected if nothing is typed. the order does not change. I would recommend doing the same for VSCode; changing the order is disorienting.

ghost · 2017-12-01T15:23:15Z

@amcasey Good to go?

amcasey · 2017-12-01T18:31:08Z

@andy-ms, have you tried it in VS? If not, I will. Otherwise, yes.

amcasey · 2017-12-01T19:04:35Z

So far, it's not working.

console.log('Hello world');

class CA { }
class CB { }

const c: CB = //caret

If I invoke completion there, the recommended completion is "CB". I actually wanted "new".
"CB" isn't pre-selected. I'm still investigating why not.

amcasey · 2017-12-01T19:28:44Z

I've confirmed that the flag is getting to Roslyn, but I have yet to figure out why it's having no effect.

amcasey · 2017-12-01T19:36:52Z

Looks like a problem in the managed component. I think you're good to go.

ghost · 2017-12-01T20:59:48Z

@amcasey Thanks!

ghost requested a review from armanio123 November 14, 2017 22:24

ghost force-pushed the completionsRecommended branch 3 times, most recently from bd04056 to bfb1538 Compare November 15, 2017 02:47

Support a 'recommended' completion entry

22123aa

ghost force-pushed the completionsRecommended branch from bfb1538 to 22123aa Compare November 15, 2017 03:02

armanio123 approved these changes Nov 16, 2017

View reviewed changes

andy-ms added 2 commits November 16, 2017 16:52

Code review

4a757e9

Merge branch 'master' into completionsRecommended

d1abfbe

ghost force-pushed the completionsRecommended branch from ccfe89d to d1abfbe Compare November 17, 2017 15:28

ghost requested a review from mjbvz November 17, 2017 23:04

Merge branch 'master' into completionsRecommended

8918891

mhegazy suggested changes Nov 20, 2017

View reviewed changes

Andy Hanson added 2 commits November 30, 2017 15:24

Merge branch 'master' into completionsRecommended

52a0b4b

Restore duplicate comments

c37427c

ghost force-pushed the completionsRecommended branch from 32ca804 to c37427c Compare December 1, 2017 00:11

mhegazy approved these changes Dec 1, 2017

View reviewed changes

mjbvz approved these changes Dec 1, 2017

View reviewed changes

ghost merged commit fd4d8ab into master Dec 1, 2017

ghost deleted the completionsRecommended branch December 1, 2017 21:00

mjbvz mentioned this pull request Dec 16, 2017

Support CompletionEntry.isRecommended from TypeScript microsoft/vscode#40325

Closed

ghost mentioned this pull request Dec 18, 2017

Support completions contextual types in more places #20768

Merged

ghost mentioned this pull request Jan 9, 2018

Support completions for enums in switch cases #16864

Closed

ghost mentioned this pull request Feb 14, 2018

Supports more locations for completions contextual types #21946

Merged

ghost mentioned this pull request Apr 2, 2018

CompletionList should be able to point to best suggestion microsoft/vscode#47102

Closed

ghost mentioned this pull request May 22, 2018

Support recommended completion for union #24326

Merged

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

Support a 'recommended' completion entry #20020

Support a 'recommended' completion entry #20020

ghost commented Nov 14, 2017 •

edited by ghost

Loading

armanio123 Nov 16, 2017

armanio123 Nov 16, 2017

ghost commented Nov 17, 2017

mjbvz commented Nov 17, 2017 •

edited

Loading

mhegazy Nov 20, 2017

mhegazy Nov 20, 2017

mhegazy Nov 20, 2017

mhegazy Nov 20, 2017

amcasey Nov 30, 2017

ghost Nov 30, 2017

amcasey Dec 1, 2017

mhegazy commented Nov 20, 2017

ghost commented Dec 1, 2017

amcasey commented Dec 1, 2017

amcasey commented Dec 1, 2017

amcasey commented Dec 1, 2017

amcasey commented Dec 1, 2017

ghost commented Dec 1, 2017

Support a 'recommended' completion entry #20020

Support a 'recommended' completion entry #20020

Conversation

ghost commented Nov 14, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Nov 17, 2017

mjbvz commented Nov 17, 2017 • edited Loading

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

mhegazy commented Nov 20, 2017

ghost commented Dec 1, 2017

amcasey commented Dec 1, 2017

amcasey commented Dec 1, 2017

amcasey commented Dec 1, 2017

amcasey commented Dec 1, 2017

ghost commented Dec 1, 2017

ghost commented Nov 14, 2017 •

edited by ghost

Loading

mjbvz commented Nov 17, 2017 •

edited

Loading