-
Notifications
You must be signed in to change notification settings - Fork 13.1k
Support completions for local named exports #37606
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
Conversation
| * | ||
| * Relevant symbols are stored in the captured 'symbols' variable. | ||
| * | ||
| * @returns true if 'symbols' was successfully populated; false otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a lie
| verify.completions({ | ||
| marker: "", | ||
| exact: ["T"] | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add tests for export { a, /*here*/}, export { a/*here*/ as /*here*/}, export { a as b, /*here*/ }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This brings up a good point: I was filtering out symbols that were already exported, but it’s valid to export the same thing several times if you rename:
export { T, T as T2, T as T3, T as T4, ... }Do you think I should eliminate the filtering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think instead of filtering you should sort text of those at lower priority ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how practical it is, but another option might be to include the as the second time it's committed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sheetalkamat how would you set the sort text here? It defaults to LocationPriority, and the other enum member names aren’t accurate, even though the values are just "1", "2", etc... IIRC adding/changing SortText required changes elsewhere. Is it ok to use OptionalMember here for names that have already been included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
symbolToSortTextMap[getSymbolId(symbol)] = SortText./whatever you want to set/;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s what I currently have, it’s just that none of the existing SortText members describe the scenario, and I think I recall that adding a new one messed something up for @uniqueiniquity one time, but he can correct me if that’s wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I would prefer you didn't add new ones.... especially new ones above GlobalsOrKeywords. The other issue we had (i.e. the value for JavaScriptIdentifiers kept changing) is resolved now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m currently just using OptionalMember which is incorrect if introspecting by enum member name, but ultimately is just "1".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sheetal's feedback all looks great, so consider this a 👍 for the overall PR when they're addressed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems sane, but I will also defer to Sheetal.
Fixes #36213