Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filter completion list according to meaning of the location #16206

Merged
merged 5 commits into from
Jun 14, 2017

Conversation

sheetalkamat
Copy link
Member

@sheetalkamat sheetalkamat commented Jun 1, 2017

Fixes #15750, #14285

@@ -954,7 +954,7 @@ var nodeServerInFile = "tests/webTestServer.ts";
compileFile(nodeServerOutFile, [nodeServerInFile], [builtLocalDirectory, tscFile], [], /*useBuiltCompiler:*/ true, { noOutFile: true, lib: "es6" });

desc("Runs browserify on run.js to produce a file suitable for running tests in the browser");
task("browserify", ["tests", builtLocalDirectory, nodeServerOutFile], function() {
task("browserify", ["tests", run, builtLocalDirectory, nodeServerOutFile], function() {
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 update the gulpfile as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no clue what to update there

Copy link
Member

Choose a reason for hiding this comment

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

I think on this line change [servicesFile] to [run, servicesFile] (or maybe [servicesFile, run]).

//// var x1;
//// /*globalValue*/

interface VeriferCompletionsInJsDoc {
Copy link
Member

Choose a reason for hiding this comment

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

typo:Verify

/////*globalValue*/
////x./*valueMember*/

interface VeriferCompletionsInJsDoc {
Copy link
Member

Choose a reason for hiding this comment

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

typo:Verify

return true;
}

if (symbol.flags & (SymbolFlags.ValueModule | SymbolFlags.NamespaceModule)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if this is a NamespaceModule, why can not we just return true?

Copy link
Member Author

Choose a reason for hiding this comment

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

Would show "m" in the completion even if it doesn't contain type or doesn't have any exported type. And user will get error with typing just m and there is nothing that can be dotted off. eg. namespace m { interface I {} }

Copy link
Contributor

Choose a reason for hiding this comment

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

if it is un-instantiated, why cannot it be referenced in a type location? if it is, i understand we need to do the work to verify.

Copy link
Member Author

Choose a reason for hiding this comment

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

because after typing var x : m. in above example there is nothing that can be shown so m is also not part of completion at var x:/*here*/

@@ -1152,7 +1239,7 @@ namespace ts.Completions {
containingNodeKind === SyntaxKind.VariableDeclarationList ||
containingNodeKind === SyntaxKind.VariableStatement ||
containingNodeKind === SyntaxKind.EnumDeclaration || // enum a { foo, |
isFunction(containingNodeKind) ||
isFunctionLikeButNotConstructor(containingNodeKind) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

why not constructors?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because that's not solely new identifier location (you can write modifiers etc)

}

function isContextTokenValueLocation(contextToken: Node) {
if (contextToken) {
Copy link
Contributor

Choose a reason for hiding this comment

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

return contextToken && contextToken.kind === SyntaxKind.TypeOfKeyword && contextToken.parent.kind === SyntaxKind.TypeQuery;

return parentKind === SyntaxKind.TypeAliasDeclaration;

case SyntaxKind.AsKeyword:
return parentKind === SyntaxKind.AsExpression;
Copy link
Contributor

Choose a reason for hiding this comment

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

what about the other type assertion syntax?

Copy link
Member Author

Choose a reason for hiding this comment

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

The TypeAssertion is anyways type node and the token < will have parent as type assertion and hence it will be type location any which ways. The tokens in as and var/property declarations are different because the parent declarations are value and hence need special handling

@mhegazy
Copy link
Contributor

mhegazy commented Jun 12, 2017

@sheetalkamat can you refresh the PR and merge

@mhegazy
Copy link
Contributor

mhegazy commented Jun 13, 2017

waiting on tests. but once merged please port to release-2.4

@sheetalkamat
Copy link
Member Author

@mhegazy the ci build passed(verified by checking out details as well) but it also shows typescript_node.6 test run failed and other two not started only. Don't know what it means.

@mhegazy mhegazy merged commit 1f16778 into master Jun 14, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Jun 14, 2017

merged manually. we need to port this to release-2.4

@mhegazy mhegazy deleted the completionWithMeaning branch June 14, 2017 01:03
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
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.

4 participants