-
Notifications
You must be signed in to change notification settings - Fork 13.2k
Completion list show correct entry for function expression and class expression #3643
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
src/services/services.ts
Outdated
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.
Look into getDeclaredName instead, including the above for the default export
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.
getDeclaredName doesn't handle function expression correctly. Talk with @mhegazy, the function needed some rewrite and I don't want to have it done in this PR. I will put a TODO here to have it change once getDeclaredName is done
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.
@DanielRosenwasser should have a fix for this in a pending review. you should synchronize.
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.
Ah right, it's in my PR. Speaking of which, can you guys review it? 😃
#3367
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.
You should be clear to use getDeclaredName
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
🌵
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.
note. you need to handle this on the managed side as well.
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.
What you mean be handling in the managed side?
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.
Probably best to discuss this in person with someone but I believe the idea is that VS needs to handle styles differently depending on what comes in to the managed side.
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.
what do you see in navbar when you do this?
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.
🌵 file a bug
|
@yuit if you pull in from |
…with certain kind, document, and text is the completion list
src/compiler/checker.ts
Outdated
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.
Why not change the logic of copySymbol?
Also, the comment for copySymbol says // Returns 'true' if we should stop processing symbols., but it returns void. Can you fix that?
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.
🌵
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.
Remove the old comment
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.
🌵
Conflicts: src/compiler/checker.ts
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.
Nice, that is cute :)
Conflicts: tests/cases/fourslash/renameLocationsForClassExpression01.ts
…t now correctly represent declared-name.
src/compiler/checker.ts
Outdated
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.
class expression listed twice
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.
Uh oh. It'd be good to have a feature that catches this.
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.
@JsonFreeman it's being tracked in #2854
|
👍 just remove the class expression label |
|
@yuit Do you still need to do all these things now that you fixed the binder? |
|
@JsonFreeman I make a change so that the service layer will get name through |
|
Great! Thanks Yui! |
|
👍 |
|
@JsonFreeman Thank you for the good discussion we had 😄 |
… completionListWithLocalName
Completion list show correct entry for function expression and class expression
Fix #3231 and for class expression case