-
Notifications
You must be signed in to change notification settings - Fork 729
Add Range to Hover #1489
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
Add Range to Hover #1489
Conversation
Co-authored-by: DanielRosenwasser <[email protected]>
Co-authored-by: DanielRosenwasser <[email protected]>
testdata/baselines/reference/fourslash/hover/JsdocLink1.baseline
Outdated
Show resolved
Hide resolved
… and remove whitespace from ranges Co-authored-by: DanielRosenwasser <[email protected]>
|
@copilot Fix whatever is broken in CI |
|
@copilot fix |
Co-authored-by: DanielRosenwasser <[email protected]>
|
@copilot Merge main and try again. Note that there now exist helpers that can do things like getting ranges for nodes like |
Merged main and refactored to use the |
|
well, copilot doesn't understand how submodules work in merge conflicts either, understandable |
Co-authored-by: jakebailey <[email protected]>
Co-authored-by: jakebailey <[email protected]>
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.
Pull Request Overview
This pull request implements a performance improvement and refactoring for the hover range calculation in the Language Server Protocol (LSP) implementation. The primary goal is to eliminate duplicate calls to getNodeForQuickInfo and ensure hover ranges accurately reflect the symbol's extent without including surrounding whitespace.
Key changes:
- Removed duplicate
getNodeForQuickInfocalls by eliminating thegetQuickInfoAndDocumentationwrapper function - Implemented direct use of
getRangeOfNodehelper for more accurate range calculation - Updated test baselines to reflect the new behavior where hover ranges now include the full identifier/symbol instead of just a single character
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 is now exactly what I expected this to look like.
|
We are both review poisoned |
Summary
Successfully merged main branch and refactored the hover range calculation:
Key changes:
getNodeForQuickInfocalls by removing thegetQuickInfoAndDocumentationwrapper functionProvideHovernow directly callsgetQuickInfoAndDocumentationForSymbolwith the pre-calculatedrangeNodel.getRangeOfNode(rangeNode, nil, nil)scannerimportLanguageServicewith receiver)The code now calls
getNodeForQuickInfoonly once per hover request, improving performance and following the same pattern used in find-all-references and other LSP features.Fixes #1488
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.