Skip to content

Conversation

ghost
Copy link

@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
// class c { method() { } b| }
return isFromObjectTypeDeclaration(location) && (location.parent as ClassElement | TypeElement).name === location
? location.parent.parent as ObjectTypeDeclaration
: tryCast(location, isClassLike);
Copy link
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 ?

return tryCast(contextToken.parent, isObjectTypeDeclaration);
default:
return isFromObjectTypeDeclaration(contextToken) &&
(isClassMemberCompletionKeyword(contextToken.kind) || isIdentifier(contextToken) && isClassMemberCompletionKeywordText(contextToken.text))
Copy link
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

@@ -114,7 +114,7 @@
////}
////class O extends B {
//// constructor(public a) {
//// },
Copy link
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
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
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
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
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
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
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?

@@ -1527,58 +1528,51 @@ namespace ts.Completions {
* Relevant symbols are stored in the captured 'symbols' variable.
*/
function tryGetClassLikeCompletionSymbols(): GlobalsSearch {
const classLikeDeclaration = tryGetClassLikeCompletionContainer(contextToken);
if (!classLikeDeclaration) return GlobalsSearch.Continue;
const decl = contextToken && tryGetObjectTypeDeclarationCompletionContainer(contextToken, location);
Copy link
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