Skip to content

Handle completions in interface / type literal similar to class#22701

Merged
3 commits merged into
masterfrom
completionsInterfaceElement
Mar 23, 2018
Merged

Handle completions in interface / type literal similar to class#22701
3 commits merged into
masterfrom
completionsInterfaceElement

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Mar 19, 2018

Fixes #22672

Previously we provided global completions inside of interfaces / type literals. Now we will only provide the readonly keyword.

@ghost ghost requested a review from sheetalkamat March 19, 2018 21:42
@ghost ghost force-pushed the completionsInterfaceElement branch from 5b2a5f6 to 60a9c87 Compare March 19, 2018 21:42
@ghost ghost force-pushed the completionsInterfaceElement branch from 60a9c87 to 7bac82f Compare March 19, 2018 22:26
Comment thread src/services/completions.ts Outdated
// class c { method() { } b| }
return isFromObjectTypeDeclaration(location) && (location.parent as ClassElement | TypeElement).name === location
? location.parent.parent as ObjectTypeDeclaration
: tryCast(location, isClassLike);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this not isObjectTypeDeclaration ?

Comment thread src/services/completions.ts Outdated
return tryCast(contextToken.parent, isObjectTypeDeclaration);
default:
return isFromObjectTypeDeclaration(contextToken) &&
(isClassMemberCompletionKeyword(contextToken.kind) || isIdentifier(contextToken) && isClassMemberCompletionKeywordText(contextToken.text))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm dont you need it to be classMemberCompletion if its call location and "readonly" in type literals and interfaces

////}
////class O extends B {
//// constructor(public a) {
//// },
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this change?


////interface a { /*interfaceValue1*/

goTo.eachMarker(() => verify.completionListIsEmpty());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you instead of deleting this have test case that verifies "aa" is not present in the completion

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

completionsInterfaceElement does basically the same thing -- verify.completionsAt will fail if there were completions we didn't list.


////interface a { f/*interfaceValue2*/

goTo.eachMarker(() => verify.completionListIsEmpty());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you instead of deleting this have test case that verifies "aa" is not present in the completion

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a test in completionsInterfaceElement just like this.

//// m(): void;
//// fo/*i*/
////}
////type T = { fo/*t*/ };
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also need test case when interface I { /marker/ } and similar type literal

////}
////type T = { fo/*t*/ };

//verify.completionsAt("i", ["readonly"]);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this commented out?

Comment thread src/services/completions.ts Outdated
function tryGetClassLikeCompletionSymbols(): GlobalsSearch {
const classLikeDeclaration = tryGetClassLikeCompletionContainer(contextToken);
if (!classLikeDeclaration) return GlobalsSearch.Continue;
const decl = contextToken && tryGetObjectTypeDeclarationCompletionContainer(contextToken, location);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like change from previous behaviour where event if contextToken was not present we would use location to decide on class location. Why is this changed ?

@ghost ghost merged commit 9557e4a into master Mar 23, 2018
@ghost ghost deleted the completionsInterfaceElement branch March 23, 2018 23:04
@microsoft microsoft locked and limited conversation to collaborators Jul 25, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant