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

[TECHDEBT]: Remove _SPECIAL_PROPERTIES hack in lsp_engine.py #1378

Open
1 task done
ericvergnaud opened this issue Dec 18, 2024 · 0 comments
Open
1 task done

[TECHDEBT]: Remove _SPECIAL_PROPERTIES hack in lsp_engine.py #1378

ericvergnaud opened this issue Dec 18, 2024 · 0 comments
Labels
enhancement New feature or request

Comments

@ericvergnaud
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues

Category of feature request

Transpile

Problem statement

Transpile Document Request is not serialized properly OOTB by lsprotocol. To work around the issue, lsp_engine extends the _SPECIAL_PROPERTIES private data structure. This is ugly and risky.

Proposed Solution

Implement a custom serialization hook.

Additional Context

No response

@ericvergnaud ericvergnaud added the enhancement New feature or request label Dec 18, 2024
gueniai added a commit that referenced this issue Jan 15, 2025
This PR implements `LSPEngine`, a `TranspileEngine` that leverages the
Language Server Protocol to launch and communicate with a pluggable
transpiler, implemented as a LSP server.

In our prototype, we used the existing LSP `CodeAction` mechanism with
`CodeActionKind.Refactor.` This design works well when using VSCode as a
client, but is not ideal for batch transpile:
- it does not provide a deterministic way to identify the command for
transpiling to databricks
- it requires 2 interactions with the server for each file (1 to fetch
the command, 1 to execute it)
- the transpile result is sent asynchronously, which complicates the
client engine's job
- the transpile errors are sent asynchronously, which complicates the
client engine's job

For those reasons, this PR chooses a different design, relying on LSP
custom capabilities.
The `LSPEngine` supports a "document/transpileToDatabricks" capability,
that the server must register during initialization.
Once the registration is done, the LSPEngine can then safely invoke that
capability, which returns all the results in a single response (changes
and errors).

This mechanism is tested successfully against a test LSPServer (which
for transpiling, simply sets the content to uppercase).

Worth noting, this PR works around a limitation (bug ?) in the
lsprotocol Python library, where custom capability messages are
incorrectly serialized. This is considered tech debt, for which an issue
has been created: #1378

Fixes #1299 
Requires #1364 
Requires #1390
Supersedes #1354

---------

Co-authored-by: Guenia Izquierdo Delgado <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant