Skip to content

Don't show the currently-completing thing at the cursor in JS files#6707

Merged
RyanCavanaugh merged 2 commits into
microsoft:masterfrom
RyanCavanaugh:fix6693
Feb 1, 2016
Merged

Don't show the currently-completing thing at the cursor in JS files#6707
RyanCavanaugh merged 2 commits into
microsoft:masterfrom
RyanCavanaugh:fix6693

Conversation

@RyanCavanaugh
Copy link
Copy Markdown
Member

Fixes #6693

@mhegazy
Copy link
Copy Markdown
Contributor

mhegazy commented Jan 28, 2016

nice 😉

@mhegazy
Copy link
Copy Markdown
Contributor

mhegazy commented Jan 28, 2016

👍

1 similar comment
@vladima
Copy link
Copy Markdown
Contributor

vladima commented Jan 28, 2016

👍

@RyanCavanaugh
Copy link
Copy Markdown
Member Author

Investigating test failure...

Comment thread src/services/services.ts
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.

Isn't this too order-dependent? What happens if you add code prior to another usage? For instance, I start out with:

if (Person.blah) {
    doSomething();
}

I'd want to also get completion for if I added a use of Person.blah before the if statement:

Person.blah/**/

if (Person.blah) {
    doSomething();
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If you have multiple uses, the value in the table becomes -1 and no filtering occurs

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.

Ah, okay. I misunderstood.

Why don't you use an enum that would make this explicit?

const name = (node as Identifier).text;
nameTable[name] = hasProperty(nameTable, name) ? NameTableState.Once : NameTableState.Many;

or at the very least, leave a nice big comment explaining what you're doing here and what -1 means.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Because we need the position in the case where it's Once.

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.

Ah, right. You're not checking the current identifier and asking "should I actually show this", you're just grabbing up identifiers. I'm not sure why I missed that.

RyanCavanaugh added a commit that referenced this pull request Feb 1, 2016
Don't show the currently-completing thing at the cursor in JS files
@RyanCavanaugh RyanCavanaugh merged commit 202c1e6 into microsoft:master Feb 1, 2016
@RyanCavanaugh RyanCavanaugh deleted the fix6693 branch February 1, 2016 18:03
@billti billti mentioned this pull request Feb 4, 2016
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 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.

5 participants