-
Notifications
You must be signed in to change notification settings - Fork 747
Generate remaining sig help tests, fix remaining bugs #2170
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
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 PR improves signature help functionality by properly handling the active parameter capability, ensuring that activeParameter is placed on SignatureInformation when the client supports it rather than on the top-level SignatureHelp. It also adds comment-in-code checking, ports backup JavaScript signature help code, and generates all remaining signature help tests.
Key Changes:
- Moved
activeParameterfromSignatureHelptoSignatureInformationbased on client capabilities - Added
inCommentchecking to bail out early when user triggers signature help in comments - Implemented JavaScript fallback signature help for untyped code
- Generated comprehensive signature help test suite
Reviewed changes
Copilot reviewed 145 out of 145 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
internal/ls/signaturehelp.go |
Core implementation of capability-aware active parameter handling and JS fallback |
| Test baseline files (multiple) | Updated baselines reflecting new active parameter placement |
| Generated test files (multiple) | New comprehensive test coverage for signature help scenarios |
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 is this one a manual test?
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 a JS test that previously used removed JSDoc syntax, like function(string, boolean=). The two manual tests are ones I fixed.
| TestAutoImportVerbatimTypeOnly1 | ||
| TestBestCommonTypeObjectLiterals | ||
| TestBestCommonTypeObjectLiterals1 | ||
| TestCalledUnionsOfDissimilarTyeshaveGoodDisplay |
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 hate when my unions of dissimilar tyes don't haveGoodDisplay
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 one is just type-ordering, right?
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.
Yep, but we loved tests with hardcoded strings (for better or for worse).
inCommentnow that we have itPlus, generate all remaining tests that mention sig help. Some new tests with unrelated issues (other LSP features) become skipped.
Fixes #1495