Skip to content

Conversation

@RyanCavanaugh
Copy link
Member

Fixes #6693

@mhegazy
Copy link
Contributor

mhegazy commented Jan 28, 2016

nice 😉

@mhegazy
Copy link
Contributor

mhegazy commented Jan 28, 2016

👍

1 similar comment
@vladima
Copy link
Contributor

vladima commented Jan 28, 2016

👍

@RyanCavanaugh
Copy link
Member Author

Investigating test failure...

Copy link
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
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
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
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
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