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

Add a new /lsp endpoint to blackd #2512

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Add a new /lsp endpoint to blackd #2512

wants to merge 1 commit into from

Conversation

zsol
Copy link
Collaborator

@zsol zsol commented Sep 26, 2021

This endpoint implements the document formatting request from the language server protocol spec: https://microsoft.github.io/language-server-protocol/specifications/specification-3-17/#textDocument_formatting
There are a few limitations to this implementation:

TODO:

  • Add changelog entry
  • Add unit tests to cover lsp.py
  • Reimplement on top of something else than aiohttp-json-rpc

This endpoint implements the document formatting request from the language server protocol spec: https://microsoft.github.io/language-server-protocol/specifications/specification-3-17/#textDocument_formatting
There are a few limitations to this implementation:
* it ignores all formatting options that are passed in
* on syntax error it returns a generic "internal server error" message
* it only supports `file://` URIs
* there's no support for initialization protocol messages: https://microsoft.github.io/language-server-protocol/specifications/specification-3-17/#initialize

TODO:

- [ ] Add changelog entry
- [ ] Add unit tests to cover lsp.py
@zsol
Copy link
Collaborator Author

zsol commented Sep 27, 2021

Let me revisit this implementation, apparently the dependency I chose is doomed: pengutronix/aiohttp-json-rpc#62

@brettcannon
Copy link
Member

@karthiknadig can you give a review?

@karthiknadig
Copy link

Thanks for adding support for the formatting request. But to fully support formatting over LSP, black should be able to handle plain text formatting. This is to support format-on-save and format-on-type, where the contents have not yet been written to the disk, so just having the path won't be enough. There are two cases there to handle, 1) the file exists on disk, but the content is not yet saved to disk. 2) User just created a new (unsaved file), the file is not yet available on disk. (but we can provide a educated guess for the uri of the directory where it might get saved, which could be used to get the formatting settings for black).

@brettcannon
Copy link
Member

Another key point about supporting the "file not on disk" scenario is that's required to support things like virtual workspaces when the content lives entirely on GitHub (i.e. what https://marketplace.visualstudio.com/items?itemName=GitHub.remotehub enables). This then bleeds into things like any chance of making Black work with github.dev via WebAssembly, etc.

@ichard26
Copy link
Collaborator

Hi what's the status on this? Is there still a pressing need for this PR now that the VSCode team have their own Black LSP server?

@brettcannon
Copy link
Member

@ichard26 I think it depends on who you want to own the server and whether others want access. I see a few options:

  1. Status quo (i.e. a language server implementation exists in https://github.com/microsoft/vscode-black-formatter for others to extract and copy as needed; it's licensed under MIT)
  2. The language server from https://github.com/microsoft/vscode-black-formatter stays in-place but we make a PyPI package for it (which we are happy to do, we just haven't done it as no one has come forward to show interest to motivate us to put in the effort 😁)
  3. The language server from https://github.com/microsoft/vscode-black-formatter is extracted out to a separate repo (which we are also happy to do, but we have the same ask about making sure the effort will be used by folks)
  4. We contribute the language server we have written upstream and continue to help maintain it
  5. This PR gets cleaned up and merged and we can look at changing our dependency to blackd in the VS Code extension

We're open to any of this as we want to do what's best for the project and community.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants