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

Integrate jupyterlab-lsp into jupyterlab #12534

Merged
merged 585 commits into from Aug 1, 2022
Merged

Conversation

trungleduc
Copy link
Member

@trungleduc trungleduc commented May 5, 2022

References

This PR improves the LSP integration in the JupyterLab frontend to make LSP features available to other extensions. It will lay the infrastructure for handling and communicating with language servers.

This PR does not contain any frontend features like code completion, code refactoring, or jump to..., it will be the subject of future work. An example of an LSP code completion provider is available here

JupyterLab will not come with any language servers, users still need to install language servers manually.

Code changes

  • On the frontend, the core elements of the jupyterlab-lsp extension that handles the language server initialization and communication are moved into the lsp package of JupyterLab.

src_diagram4

Diagram of components taken into JupyterLab

  • lsp-extension package is created to provide tokens that allow other extensions to interact with the language servers:
    - ILSPDocumentConnectionManager: the token used to manage the connection to language servers for the notebook and file editor.
    - ILSPFeatureManager: the token used to register features with language servers.
    - ILSPCodeExtractorsManager: the token used to manage the code extractors, this will allow the creation of multiple language server connections on one notebook.

  • The backend parts, for now, are provided by jupyter-lsp package.

User-facing changes

Running language server panel

A new language server section is added to the running panel. If the language server for the language of the opened notebook is available, JupyterLab will create a connection to this server for the notebook. The language server section will show the running server and allow users to terminate these servers.

lsp2

Setting interface for the language server

JupyterLab will detect automatically the installed language servers and will propose the settings for these servers in the settings UI.
lsp3

Remaining tasks

I will address the last 2 tasks in the follow-up PR.

Backwards-incompatible changes

N/A

cc @krassowski @bollwyvl

Binder

krassowski and others added 30 commits December 12, 2020 19:30
Refactor how the notebook changes are picked up, events bound and unbound
…finition

Refactor jump-to functionality, remove unused code
which were not correctly scoped by path by switching away from
id_path which was refactored to not contain the actual file name
to uri which does contain the file name and path
@trungleduc
Copy link
Member Author

@krassowski, we a finalizing this PR and the remaining discussion is the scope of unstable APIs. I tagged all newly added tokens and APIs as alpha (783056f). Do we need to aware of any old APIs coming from the extension?

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.

Excited to see this so close to the finish line. Leaving some feedback in answer to the question about old interfaces: basically lsp-ws-connection has a lot of deprecated stuff which we should cut to avoid bringing useless dependencies (like events) and dev dependencies.

Also, I noticed that standard copyright headers are missing:

// Copyright (c) Jupyter Development Team.
// Distributed under the terms of the Modified BSD License.

and sometimes there are headers in another format. I would suggest to use the standard lab format for consitency, it will be easier for us if we decide to grep-change them later.

packages/fileeditor/src/fileeditorlspadapter.ts Outdated Show resolved Hide resolved
packages/fileeditor/src/fileeditorlspadapter.ts Outdated Show resolved Hide resolved
packages/lsp-extension/schema/plugin.json Outdated Show resolved Hide resolved
packages/lsp-extension/schema/plugin.json Show resolved Hide resolved
packages/lsp/src/connection.ts Outdated Show resolved Hide resolved
"@lumino/signaling": "^1.10.1",
"codemirror": "~5.61.0",
"lodash.mergewith": "^4.6.1",
"lsp-ws-connection": "~0.7.0",
Copy link
Member

Choose a reason for hiding this comment

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

I think we should remove this dependency, it was getting axed anyways (I think this is incorrectly ticked as done in the PR description). It only provides registerServerCapability and unregisterServerCapability and LspWsConnection; let's merge LspWsConnection into LSPConnection removing @deprecated methods (everything which uses events.EventEmitter or old get/send API).

@fcollonval
Copy link
Member

Regarding the license header, I'll try to add an GitHub action to reach homogeneity. I found the promising: https://github.com/marketplace/actions/license-eye-header

@trungleduc
Copy link
Member Author

Thank @krassowski for your review, I removed lsp-ws-connection in 1ccb751. I did not merge LspWsConnection into LSPConnection since I want to separate vendor codes.

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.

Only two minor comments otherwise the code looks good.

packages/lsp/src/tokens.ts Show resolved Hide resolved
packages/lsp/src/tokens.ts Show resolved Hide resolved
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.

Some quick coffee-break comments, will take another look later.

packages/lsp/src/ws-connection/types.ts Outdated Show resolved Hide resolved
packages/lsp/src/ws-connection/types.ts Outdated Show resolved Hide resolved
@SylvainCorlay
Copy link
Member

Hey @krassowski let us know if we could address more things in follow up PRs. It would be really nice to get this in (in a pre-release) so that we could start working on the follow-up changes in the extension. We will most certainly find more things to fix as we adapt jupyterlab-lsp to use this code, and codemirror 6.

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.

I'm happy for it to be merged and iterated on later (though let's address the two minor things from my previous review so that they are not lost once we merge).

@SylvainCorlay
Copy link
Member

Thanks ! Lets fix these up an iterate!

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 a lot @trungleduc

@fcollonval fcollonval merged commit a650dd8 into jupyterlab:master Aug 1, 2022
@trungleduc trungleduc mentioned this pull request Aug 4, 2022
4 tasks
@krassowski krassowski mentioned this pull request Oct 14, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet