-
Notifications
You must be signed in to change notification settings - Fork 10
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
Wires signature help to codemirror #157
Conversation
ncordon
commented
Dec 18, 2023
11a788c
to
6ad77e2
Compare
2b093f5
to
ebd7b4e
Compare
af6b099
to
e4fdc01
Compare
8765404
to
f47df26
Compare
let result: Tooltip[] = []; | ||
const schema = config.schema; | ||
const tokens = parserWrapper.parsingResult.tokens; | ||
const lastToken = tokens.filter((token) => token.channel == 0).at(-2); |
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.
Is the stopNode
available here? It feels like that could be easier to read
findParent(tree.stopNode, (parent) => | ||
isOpenBracket | ||
? isMethodNameOrExpressionName(parent) | ||
: isMethodName(parent), |
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.
Would it make sense for this logic to live in the language-support
package?
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'd rather not do that for sake of consistency. The trigger characters are configured at language server level, this means the language support would be able to trigger signature help for an unfinished first argument (we haven't reached a separator):
CALL apoc.do.when(true
but the language server will only call the functionality when we hit a trigger character (
, ,
or )
.
Also someone may want to build an application where if they hit a certain keystroke combination, the signature help is displayed again, not just at a trigger character.
My point is I can move this into a function to the language support but I still need to call it from here.
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.
Let's leave it as is then
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.
Make it work only for the trigger characters, not inspecting the tree
const pos = state.selection.main.head; | ||
const tree = parserWrapper.parsingResult; | ||
const isOpenBracket = lastToken.text === '('; | ||
const isSeparator = lastToken.text === ','; |
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 was expecting this to work even when the )
was present:
CleanShot.2024-01-17.at.10.57.17.mp4
Is it a known limitation, we can addressing it in a follow up PR if it's hard to implement
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've added a prop to the editor offset
to be able to place the cursor somewhere that's not the end and added tests for the case at hand.
What doesn't work is still if you go back in the query and want the signature help triggered
d656616
to
a353c04
Compare
a353c04
to
f30cb0c
Compare
f30cb0c
to
5d87c47
Compare
const triggerCharacter = getTriggerCharacter(query, position); | ||
|
||
if (triggerCharacter === '(' || triggerCharacter === ',') { | ||
const queryUntilPosition = query.slice(0, position); |
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 not great because we'll reparse inside the signature help, but it's what VSCode is doing at the minute. I'll improve for both wirings in another pr.
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.
💯💯
It'd be nice with a changeset for this as well! |