-
Notifications
You must be signed in to change notification settings - Fork 31
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
feat: Add signatureHelp and hover providers to monaco #1178
feat: Add signatureHelp and hover providers to monaco #1178
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1178 +/- ##
==========================================
- Coverage 44.48% 44.19% -0.29%
==========================================
Files 437 448 +11
Lines 32944 33424 +480
Branches 8304 8403 +99
==========================================
+ Hits 14654 14771 +117
- Misses 18241 18603 +362
- Partials 49 50 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 52 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
https://code.visualstudio.com/shortcuts/keyboard-shortcuts-macos.pdf from: https://code.visualstudio.com/docs/getstarted/keybindings#_keyboard-shortcuts-reference I wonder if it would be nice to expose these in a non-editable fashion in the settings menu or something? |
I need to test this with enterprise still to ensure nothing breaks. I don't think anything will, but I might need to add back quotes in the completion item triggers. We trigger completion in |
signatureHelpTriggerCharacters: ['(', ','], | ||
} | ||
); | ||
if (session.getSignatureHelp) { |
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.
Side note, I thought we needed strict booleans, e.g. if (session.getSigHelp != null)
. Would help identify cases where we just put a method name instead of calling it, e.g. if (session.getMyBoolean)
vs. if(session.getMyBoolean())
Not related to this change though.
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.
The default rules allow it because function does not have a falsy context. The rule defaults are to avoid if (str)
if str is nullable since that condition covers both null and empty string
If you want to open a ticket to change it, the scenarios that are allowed by default are allowString
, allowNumber
, and allowNullableObject
https://typescript-eslint.io/rules/strict-boolean-expressions/#options
Corresponds with deephaven/deephaven-core#3607
This adds signatureHelp and hover providers if the JS API supports the corresponding requests
SignatureHelp is provided when you type a method and then type the
(
character. There's also probably a keybinding in monaco to trigger it, but I'm not sure what it is.Hover provided when you hover anything
Needs to use my core branch to test. After pulling that branch, will need to build/install the Python server using the following commands
Then start using one of these commands. Groovy provided just to show it doesn't break anything in Groovy. There is no autocomplete in Python notebooks if you are using a Groovy worker.