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.

Sign up for GitHub

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
Merged

feat: Add signatureHelp and hover providers to monaco #1178

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a51796e
Add signature help and hover providers
mattrunyon Mar 27, 2023
cbb36ad
Add css comment
mattrunyon Mar 27, 2023
e0873dc
Address review comments
mattrunyon Mar 29, 2023
7f0be17
Remove trigger character
mattrunyon Mar 29, 2023
615f275
Update comment
mattrunyon Mar 29, 2023
9794715
Add more unit tests
mattrunyon Mar 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions __mocks__/dh-core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1418,6 +1418,12 @@ class IdeSession extends DeephavenObject {
getCompletionItems() {
return Promise.resolve([]);
}
getSignatureHelp() {
return Promise.resolve([]);
}
getHover() {
return Promise.resolve({});
}
closeDocument() {}

newTable(columnNames, types, data, userTimeZone) {
Expand Down
115 changes: 114 additions & 1 deletion packages/console/src/monaco/MonacoProviders.test.tsx
View file
Open in desktop
mattrunyon marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ it('registers/deregisters completion provider properly', () => {
expect(disposable.dispose).toHaveBeenCalledTimes(1);
});

it('provides completion items properly', async () => {
// Enterprise provides no documentation. Community provides MarkupContent
it('provides completion items with no documentation object', async () => {
const disposable = { dispose: jest.fn() };
const newText = 'test';
const myRegister = jest.fn(() => disposable);
Expand Down Expand Up @@ -92,6 +93,82 @@ it('provides completion items properly', async () => {
expect(suggestions[0]).toMatchObject({
insertText: newText,
label: newText,
documentation: undefined,
});
});

it('provides completion items properly with documentation object', async () => {
const disposable = { dispose: jest.fn() };
const newText = 'test';
const myRegister = jest.fn(() => disposable);
monaco.languages.registerCompletionItemProvider = myRegister;
const items = [
{
label: newText,
kind: 0,
textEdit: {
range: {
start: { line: 5, character: 30 },
end: { line: 10, character: 60 },
},
text: newText,
},
documentation: {
value: 'markdown',
kind: 'markdown',
},
},
{
label: `${newText}2`,
kind: 0,
textEdit: {
range: {
start: { line: 5, character: 30 },
end: { line: 10, character: 60 },
},
text: `${newText}2`,
},
documentation: {
value: 'plaintext',
kind: 'plaintext',
},
},
];
const promiseItems = Promise.resolve(items);
const language = DEFAULT_LANGUAGE;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const session = new (dh as any).IdeSession(language);
session.getCompletionItems = jest.fn(() => promiseItems);

const model = { uri: { path: 'test' }, getVersionId: jest.fn(() => 1) };
makeProviders(language, session, model);
const position = { lineNumber: 1, column: 1 };
expect(myRegister).toHaveBeenCalledTimes(1);
expect.assertions(5);

const calls: {
provideCompletionItems: (
model: unknown,
position: unknown
) => Promise<{ suggestions: unknown[] }>;
}[] = myRegister.mock.calls[0];
const fn = calls[1];

const resultItems = await fn.provideCompletionItems(model, position);
expect(session.getCompletionItems).toHaveBeenCalled();
const { suggestions } = resultItems;
expect(suggestions.length).toBe(items.length);
expect(suggestions[0]).toMatchObject({
insertText: newText,
label: newText,
documentation: {
value: 'markdown',
},
});
expect(suggestions[1]).toMatchObject({
insertText: `${newText}2`,
label: `${newText}2`,
documentation: 'plaintext',
});
});

Expand All @@ -113,6 +190,24 @@ it('registers/deregisters signature help provider properly', () => {
expect(disposable.dispose).toHaveBeenCalledTimes(1);
});

it('does not register signature help if it is not available', () => {
const disposable = { dispose: jest.fn() };

monaco.languages.registerSignatureHelpProvider = jest.fn(() => disposable);

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const session = new (dh as any).IdeSession(DEFAULT_LANGUAGE);
session.getSignatureHelp = undefined;

const wrapper = makeProviders(DEFAULT_LANGUAGE, session);

expect(monaco.languages.registerSignatureHelpProvider).not.toHaveBeenCalled();

wrapper.unmount();

expect(disposable.dispose).not.toHaveBeenCalled();
});

it('provides signature help properly', async () => {
const disposable = { dispose: jest.fn() };
const newText = 'test';
Expand Down Expand Up @@ -189,6 +284,24 @@ it('registers/deregisters hover provider properly', () => {
expect(disposable.dispose).toHaveBeenCalledTimes(1);
});

it('does not register hover if it is not available', () => {
const disposable = { dispose: jest.fn() };

monaco.languages.registerHoverProvider = jest.fn(() => disposable);

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const session = new (dh as any).IdeSession(DEFAULT_LANGUAGE);
session.getHover = undefined;

const wrapper = makeProviders(DEFAULT_LANGUAGE, session);

expect(monaco.languages.registerHoverProvider).not.toHaveBeenCalled();

wrapper.unmount();

expect(disposable.dispose).not.toHaveBeenCalled();
});

it('provides hover info properly', async () => {
const disposable = { dispose: jest.fn() };
const newText = 'test';
Expand Down
116 changes: 67 additions & 49 deletions packages/console/src/monaco/MonacoProviders.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import { PureComponent } from 'react';
import * as monaco from 'monaco-editor';
import Log from '@deephaven/log';
import { IdeSession } from '@deephaven/jsapi-shim';
import { DocumentRange, IdeSession, Position } from '@deephaven/jsapi-shim';

const log = Log.module('MonacoCompletionProvider');

Expand All @@ -23,10 +23,13 @@ class MonacoProviders extends PureComponent<
> {
/**
* Converts LSP CompletionItemKind to Monaco CompletionItemKind
mattrunyon marked this conversation as resolved.
Show resolved Hide resolved
* Defaults to Variable if no LSP kind was provided
* https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#completionItemKind
*
* @param kind The LSP kind
* @returns Monaco kind
*/
static lspToMonacoKind(kind: number): number {
static lspToMonacoKind(kind: number | undefined): number {
const monacoKinds = monaco.languages.CompletionItemKind;
switch (kind) {
case 1:
Expand Down Expand Up @@ -84,6 +87,40 @@ class MonacoProviders extends PureComponent<
}
}

/**
* Converts an LSP document range to a monaco range
* Accounts for LSP indexing from 0 and monaco indexing from 1
*
* @param range The LSP document range to convert
* @returns The corresponding monaco range
*/
static lspToMonacoRange(range: DocumentRange): monaco.IRange {
const { start, end } = range;

// Monaco expects the columns/ranges to start at 1. LSP starts at 0
return {
startLineNumber: start.line + 1,
startColumn: start.character + 1,
endLineNumber: end.line + 1,
endColumn: end.character + 1,
};
}

/**
* Converts a monaco position to an LSP position
* Accounts for LSP indexing from 0 and monaco indexing from 1
*
* @param position The monaco position
* @returns The corresponding LSP position
*/
static monacoToLspPosition(position: monaco.IPosition): Position {
mattrunyon marked this conversation as resolved.
Show resolved Hide resolved
// Monaco 1-indexes Position. LSP 0-indexes Position
return {
line: position.lineNumber - 1,
character: position.column - 1,
};
}

constructor(props: MonacoProviderProps) {
super(props);

Expand All @@ -93,30 +130,34 @@ class MonacoProviders extends PureComponent<
}

componentDidMount(): void {
const { language } = this.props;
const { language, session } = this.props;

this.registeredCompletionProvider = monaco.languages.registerCompletionItemProvider(
language ?? '',
language,
{
provideCompletionItems: this.handleCompletionRequest,
triggerCharacters: ['.'],
triggerCharacters: ['.', '"', "'"],
}
);

this.registeredSignatureProvider = monaco.languages.registerSignatureHelpProvider(
language ?? '',
{
provideSignatureHelp: this.handleSignatureRequest,
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

this.registeredSignatureProvider = monaco.languages.registerSignatureHelpProvider(
language,
{
provideSignatureHelp: this.handleSignatureRequest,
signatureHelpTriggerCharacters: ['(', ','],
}
);
}

this.registeredHoverProvider = monaco.languages.registerHoverProvider(
language ?? '',
{
provideHover: this.handleHoverRequest,
}
);
if (session.getHover) {
this.registeredHoverProvider = monaco.languages.registerHoverProvider(
language,
{
provideHover: this.handleHoverRequest,
}
);
}
}

componentWillUnmount(): void {
Expand Down Expand Up @@ -146,22 +187,17 @@ class MonacoProviders extends PureComponent<
uri: `${model.uri}`,
version: model.getVersionId(),
},
// Monaco 1-indexes Position. LSP 0-indexes Position
position: {
line: position.lineNumber - 1,
character: position.column - 1,
},
position: MonacoProviders.monacoToLspPosition(position),
context,
};

const completionItems = session.getCompletionItems(params);
log.debug('Requested completion items', params);

const monacoCompletionItems = completionItems
.then(items => {
log.debug('Completion items received: ', params, completionItems);
log.debug('Completion items received: ', params, items);

// Annoying that the LSP protocol returns completion items with a range that's slightly different than what Monaco expects
// Need to remap the items here
const suggestions = items.map(item => {
const {
label,
Expand All @@ -174,16 +210,6 @@ class MonacoProviders extends PureComponent<
insertTextFormat,
} = item;

const { start, end } = textEdit.range;

// Monaco expects the columns/ranges to start at 1.
const range = {
startLineNumber: start.line + 1,
startColumn: start.character + 1,
endLineNumber: end.line + 1,
endColumn: end.character + 1,
};

return {
label,
kind: MonacoProviders.lspToMonacoKind(kind),
Expand All @@ -199,7 +225,7 @@ class MonacoProviders extends PureComponent<
// semantically equivalent to monaco's insertTextRules===4.
// Why microsoft is using almost-but-not-LSP apis is beyond me....
insertTextRules: insertTextFormat === 2 ? 4 : insertTextFormat,
range,
range: MonacoProviders.lspToMonacoRange(textEdit.range),
};
});

Expand All @@ -216,7 +242,6 @@ class MonacoProviders extends PureComponent<
return monacoCompletionItems;
}

// eslint-disable-next-line class-methods-use-this
handleSignatureRequest(
model: monaco.editor.ITextModel,
position: monaco.Position,
Expand Down Expand Up @@ -244,15 +269,12 @@ class MonacoProviders extends PureComponent<
uri: `${model.uri}`,
version: model.getVersionId(),
},
// Monaco 1-indexes Position. LSP 0-indexes Position
position: {
line: position.lineNumber - 1,
character: position.column - 1,
},
position: MonacoProviders.monacoToLspPosition(position),
context,
};

const signatureItems = session.getSignatureHelp(params);
log.debug('Requested signature help', params);

const monacoSignatures = signatureItems
.then(items => {
Expand Down Expand Up @@ -297,7 +319,6 @@ class MonacoProviders extends PureComponent<
return monacoSignatures;
}

// eslint-disable-next-line class-methods-use-this
handleHoverRequest(
model: monaco.editor.ITextModel,
position: monaco.Position
Expand All @@ -312,14 +333,11 @@ class MonacoProviders extends PureComponent<
uri: `${model.uri}`,
version: model.getVersionId(),
},
// Monaco 1-indexes Position. LSP 0-indexes Position
position: {
line: position.lineNumber - 1,
character: position.column - 1,
},
position: MonacoProviders.monacoToLspPosition(position),
};

const hover = session.getHover(params);
log.debug('Requested hover', params);

const monacoHover = hover
.then(hoverItem => {
Expand Down