GeezerVison

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

GeezerVison is showing github.com — Support a 'recommended' completion entry · Pull Request #20020 · microsoft/TypeScript

Conversation

Deleted user (ghost) commented Nov 14, 2017 •

edited by ghost

Loading

Uh oh!

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

Copy link

Copy Markdown

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

ghost requested a review from Armando Aguirre (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

Armando Aguirre (armanio123) approved these changes

Nov 16, 2017

View reviewed changes

Comment thread

src/services/completions.ts Outdated

		!!(localSymbol.flags & SymbolFlags.ExportValue) && checker.getExportSymbolOfSymbol(localSymbol) === recommendedCompletion;
		}

		function trueOrUndef(b: boolean): true \| undefined {

Armando Aguirre (armanio123) Nov 16, 2017

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.

Avoid abbreviations.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

src/services/completions.ts Outdated


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

Armando Aguirre (armanio123) Nov 16, 2017

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

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

Deleted user (ghost) commented Nov 17, 2017

Copy link

Copy Markdown

Author

Should probably get review from Matt Bierner (@mjbvz)

All reactions

Sorry, something went wrong.

Uh oh!

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

ghost requested a review from Matt Bierner (mjbvz)

November 17, 2017 23:04


          Merge branch 'master' into completionsRecommended

Matt Bierner (mjbvz) commented Nov 17, 2017 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

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?

All reactions

Sorry, something went wrong.

Uh oh!

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

Mohamed Hegazy (mhegazy) suggested changes

Nov 20, 2017

View reviewed changes

Comment thread

src/compiler/checker.ts

		case SyntaxKind.AmpersandAmpersandToken:
		case SyntaxKind.CommaToken:
		return node === right ? getContextualType(binaryExpression) : undefined;
		case SyntaxKind.EqualsEqualsEqualsToken:

Mohamed Hegazy (mhegazy) Nov 20, 2017

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

src/compiler/checker.ts

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

Mohamed Hegazy (mhegazy) Nov 20, 2017

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.

this one too

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

src/services/types.ts

		kindModifiers: string; // see ScriptElementKindModifier, comma separated
		kindModifiers: string;
		sortText: string;
		/**

Mohamed Hegazy (mhegazy) Nov 20, 2017

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 is this comment removed

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

src/services/types.ts

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

Mohamed Hegazy (mhegazy) Nov 20, 2017

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.

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 Andrew Casey (@amcasey) about this too.

Sorry, something went wrong.

Uh oh!

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

All reactions

Andrew Casey (amcasey) Nov 30, 2017

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'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.

Sorry, something went wrong.

Uh oh!

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

All reactions

Deleted user (ghost) Nov 30, 2017

Copy link

Copy Markdown

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

Andrew Casey (amcasey) Dec 1, 2017

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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

Mohamed Hegazy (mhegazy) commented Nov 20, 2017

Copy link

Copy Markdown

Contributor

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

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

Mohamed Hegazy (mhegazy) approved these changes

Dec 1, 2017

View reviewed changes

Deleted user (ghost) commented Dec 1, 2017

Copy link

Copy Markdown

Author

Andrew Casey (@amcasey) Good to go?

All reactions

Sorry, something went wrong.

Uh oh!

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

Andrew Casey (amcasey) commented Dec 1, 2017

Copy link

Copy Markdown

Member

Andy (Andrewkraft) (@Andy-MS), have you tried it in VS? If not, I will. Otherwise, yes.

All reactions

Sorry, something went wrong.

Uh oh!

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

Andrew Casey (amcasey) commented Dec 1, 2017

Copy link

Copy Markdown

Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

Matt Bierner (mjbvz) approved these changes

Dec 1, 2017

View reviewed changes

Andrew Casey (amcasey) commented Dec 1, 2017

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

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

Andrew Casey (amcasey) commented Dec 1, 2017

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

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

Deleted user (ghost) commented Dec 1, 2017

Copy link

Copy Markdown

Author

Andrew Casey (@amcasey) Thanks!

All reactions

Sorry, something went wrong.

Uh oh!

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

ghost merged commit fd4d8ab into master

Dec 1, 2017

ghost deleted the completionsRecommended branch

December 1, 2017 21:00

Matt Bierner (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

Merged

Microsoft (microsoft) locked and limited conversation to collaborators

Jun 14, 2018

This pull request was closed.

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.

4 participants

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.

Conversation

Deleted user (ghost) commented Nov 14, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Deleted user (ghost) commented Nov 17, 2017

Uh oh!

Matt Bierner (mjbvz) commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Mohamed Hegazy (mhegazy) commented Nov 20, 2017

Uh oh!

Deleted user (ghost) commented Dec 1, 2017

Uh oh!

Andrew Casey (amcasey) commented Dec 1, 2017

Uh oh!

Andrew Casey (amcasey) commented Dec 1, 2017

Uh oh!

Andrew Casey (amcasey) commented Dec 1, 2017

Uh oh!

Andrew Casey (amcasey) commented Dec 1, 2017

Uh oh!

Deleted user (ghost) commented Dec 1, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Deleted user (ghost) commented Nov 14, 2017 •

edited by ghost

Loading

Uh oh!

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

Matt Bierner (mjbvz) commented Nov 17, 2017 •

edited

Loading

Uh oh!

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