Skip to content
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

Merged
merged 6 commits into from
Mar 29, 2023

Conversation

mattrunyon
Copy link
Collaborator

@mattrunyon mattrunyon commented Mar 27, 2023

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

./gradlew :py-server:assemble :py-embedded-server:assemble
pip install --force-reinstall py/server/build/wheel/deephaven_core-0.23.0-py3-none-any.whl py/embedded-server/build/wheel/deephaven_server-0.23.0-py3-none-any.whl

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.

./gradlew server-jetty-app:run # Python
START_OPTS="-Ddeephaven.console.type=groovy" ./gradlew server-jetty-app:run # Groovy

@mattrunyon mattrunyon requested a review from mofojed March 27, 2023 21:54
@mattrunyon mattrunyon self-assigned this Mar 27, 2023
@codecov
Copy link

codecov bot commented Mar 27, 2023

Codecov Report

Merging #1178 (9794715) into main (362928f) will decrease coverage by 0.29%.
The diff coverage is 54.05%.

@@            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     
Flag Coverage Δ
unit 44.19% <54.05%> (-0.29%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/console/src/notebook/ScriptEditor.tsx 0.00% <0.00%> (ø)
packages/console/src/monaco/MonacoProviders.tsx 54.10% <54.10%> (ø)
packages/console/src/ConsoleInput.tsx 67.69% <100.00%> (ø)

... 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.

packages/console/src/monaco/MonacoProviders.tsx Outdated Show resolved Hide resolved
packages/console/src/monaco/MonacoProviders.tsx Outdated Show resolved Hide resolved
packages/console/src/monaco/MonacoProviders.tsx Outdated Show resolved Hide resolved
packages/console/src/monaco/MonacoProviders.tsx Outdated Show resolved Hide resolved
packages/console/src/monaco/MonacoProviders.tsx Outdated Show resolved Hide resolved
packages/console/src/monaco/MonacoProviders.test.tsx Outdated Show resolved Hide resolved
@dsmmcken
Copy link
Contributor

dsmmcken commented Mar 28, 2023

There's also probably a keybinding in monaco to trigger it, but I'm not sure what it is.

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?
microsoft/monaco-editor#823 (comment)

@mattrunyon
Copy link
Collaborator Author

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 db.t(", but at one point I had some weird completion items in DHC when triggered on quote. Mostly paths related to where Python was on the server

@mattrunyon mattrunyon requested a review from mofojed March 29, 2023 18:06
mofojed
mofojed previously approved these changes Mar 29, 2023
signatureHelpTriggerCharacters: ['(', ','],
}
);
if (session.getSignatureHelp) {
Copy link
Member

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.

Copy link
Collaborator Author

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

@mattrunyon mattrunyon merged commit f1f3abf into deephaven:main Mar 29, 2023
@mattrunyon mattrunyon deleted the autocorrect-improvements branch March 29, 2023 21:25
@github-actions github-actions bot locked and limited conversation to collaborators Mar 29, 2023
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants