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

Adds missing parameters to ICompleterProvider API #14809

Merged
merged 7 commits into from Jul 19, 2023

Conversation

hbcarlos
Copy link
Member

References

Fixes #14806

Code changes

see #14806

User-facing changes

N/A

Backwards-incompatible changes

N/A

@hbcarlos hbcarlos added the bug label Jul 10, 2023
@hbcarlos hbcarlos requested a review from krassowski July 10, 2023 14:27
@hbcarlos hbcarlos self-assigned this Jul 10, 2023
@jupyterlab-probot
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@hbcarlos
Copy link
Member Author

Hey @krassowski, should we change the IProviderReconciliator and the logic to pass the necessary arguments, or does jupyterlab-lsp reimplements the handler and everything?

export interface IProviderReconciliator {
/**
* Fetch response from multiple providers, If a provider can not return
* the response for a completer request before timeout,
* the result of this provider will be ignore.
*
* @param {CompletionHandler.IRequest} request - The completion request.
*/
fetch(
request: CompletionHandler.IRequest
): Promise<CompletionHandler.ICompletionItemsReply | null>;
/**
* Check if completer should make request to fetch completion responses
* on user typing. If the provider with highest rank does not have
* `shouldShowContinuousHint` method, a default one will be used.
*
* @param completerIsVisible - The visible status of completer widget.
* @param changed - CodeMirror changed argument.
*/
shouldShowContinuousHint(
completerIsVisible: boolean,
changed: SourceChange
): boolean;
}

this._autoCompletion &&
(this._reconciliator as IProviderReconciliator)
.shouldShowContinuousHint &&
(this._reconciliator as IProviderReconciliator).shouldShowContinuousHint(
this.completer.isVisible,
changed
)
) {
void this._makeRequest(editor.getCursorPosition());
}

return this._reconciliator
.fetch(request)

@krassowski
Copy link
Member

If we are changing the interface we should also implement the logic here. It will be useful beyond LSP implementation (for other completion providers). In the current draft the LSP implementation no longer needs to swap the completion handler (which was a nightmare previously), so hopefully we won't need to do this again.

Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! One minor thing: a tests checking that trigger gets passed down would be useful here (because trigger is optional typing will not save us if someone deletes it accidentally in a refactor).

packages/completer/src/tokens.ts Outdated Show resolved Hide resolved
hbcarlos and others added 2 commits July 13, 2023 10:03
Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>
@hbcarlos hbcarlos requested a review from krassowski July 13, 2023 13:14
@krassowski krassowski added this to the 4.0.x milestone Jul 18, 2023
Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>
Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @hbcarlos

@fcollonval
Copy link
Member

CI failure are not related

@fcollonval fcollonval merged commit 6c261cb into jupyterlab:main Jul 19, 2023
74 of 79 checks passed
@fcollonval
Copy link
Member

@meeseeksdev please backport to 4.0.x

meeseeksmachine pushed a commit to meeseeksmachine/jupyterlab that referenced this pull request Jul 19, 2023
krassowski pushed a commit that referenced this pull request Jul 19, 2023
…#14848)

Co-authored-by: Carlos Herrero <26092748+hbcarlos@users.noreply.github.com>
@hbcarlos hbcarlos deleted the fix_#14806 branch July 31, 2023 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

shouldShowContinuousHint needs context, fetch needs reason
3 participants