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

Black over Language Server Protocol and VS Code extension #2883

Open
karthiknadig opened this issue Feb 15, 2022 · 10 comments
Open

Black over Language Server Protocol and VS Code extension #2883

karthiknadig opened this issue Feb 15, 2022 · 10 comments
Labels
C: integrations Editor plugins and other integrations C: maintenance Related to project maintenance, e.g. CI, testing, policy changes, releases S: needs discussion Needs further hashing out before ready for implementation (on desirability, feasibility, etc.) T: enhancement New feature or request

Comments

@karthiknadig
Copy link

karthiknadig commented Feb 15, 2022

The Python team for VS Code has been working on putting various tools we support behind LSP and breaking them out into their own extensions This work is now far enough along that we wanted to share the extension prototypes for your tool with you and see if you had any interest in owning the extension or the Python-based LSP server backing it yourself?

If you don't want to take on that responsibility that's totally fine and understandable! If you want to leave the extension to us we would then publish the extension on the VS Code Marketplace ourselves. We also plan to publish the extension as open source and support the extension for as long as we felt it made sense based on usage numbers. But due to the difficulty of migrating extension ownership, we wanted to ask early whether you wanted to own the extension from the start? If you did want to own the extension we would give you the code, help you publish to the VS Code Marketplace, and do anything else we can to help you be successful with this.

We have attached a zip file to this issue containing a prototype extension to give you an idea of what we are planning. You can download it, extract the .vsix file, and then install the VSIX to try it out python-black.zip. This prototype version was built with python 3.10 and bundles black v22.1.0 (as fallback if nothing is found in your environment).

To be very clear, there's no pressure or expectations from us. We just want to make sure you as a project had an opportunity to own this extension instead of us from the get-go before the extension gained traction.

@karthiknadig karthiknadig added the T: enhancement New feature or request label Feb 15, 2022
@karthiknadig
Copy link
Author

For testing this you can follow these steps, after installing. Suppose you have code like this:

import unittest
class TestSum2(unittest.TestCase):
    def test_sum2(self):
        self.assertEqual(sum([1, 2, 3]), 6, "Should be 6")

    def test_sum2_tuple(self):
        self.assertEqual(sum((1, 2, 2)), 6, "Should be 6")
if __name__ == "__main__":
    unittest.main()

Right click in the document and select "Format Document With ...":
image

You should see two formatters, Select "Black":
image

That is it!

If you want this to happen automatically, set Black as the default formatter, and enable format on save. Then on save the content will be formatted.

@ichard26 ichard26 added C: integrations Editor plugins and other integrations C: maintenance Related to project maintenance, e.g. CI, testing, policy changes, releases labels Feb 17, 2022
@ichard26
Copy link
Collaborator

ichard26 commented Feb 18, 2022

Hey thanks for reaching out to us! What follows are my personal comments, other co-maintainers may have different thoughts :)

First a few initial questions about the LSP server and the extension:

  • What are the benefits of this Black extension over the native support in the core Python VS Code extension? I would have expected the Python extension to transition to integrating with all of its formatting providers via LSP instead.

  • Is the team planning to publish the LSP and extension as a single project or separately? I ask because the answer will decide whether Add a new /lsp endpoint to blackd #2512 by @zsol will be relevant

[...] owning the extension or the Python-based LSP server backing it yourself?

My initial reaction is that I would prefer not owning & maintaining the LSP server or extension ourselves since it would require stretching the already limited maintenance resources this project has. The main sticking point is that I'd guess outside contributions to the open source project will (probably) require signing a CLA with Microsoft. Not a big deal for me, but I'd like to flag it up in this discussion just in case others feel more strongly.

I do have a few important things to discuss / note if we do go down this route:

  • Will the production build of the extension bundle Black? If so, I would appreciate that an effort is made to keep it reasonably up to date. Also, given bundling the mypyc binaries obviously wouldn't be feasible, it *may* be worth it to document that manually installing Black in the environment can further improve responsiveness. Can't imagine it would be noticeable on smaller files, but it could be on larger ones.

  • What APIs will it require? Black still doesn't have an official stable API and while breakage wouldn't be the end of the world, I'd like to avoid it. It'd also help us figure out what we should include when we get around to defining one.

Although, I'm fine if we chose to own the LSP server itself since it's in Python haha

We have attached a zip file to this issue [...]

Nice! I love how responsive and fast it is! ... although that makes the logs confusing since they appear to be showing Black being run at the command line (which is always going to be relatively slow). Are they just for show since I'd assume the LSP server is actually powering the command which interacts with Black's unofficial APIs (bypassing the import time cost).

black --stdin-filename /home/ichard26/programming/webdev/blog/testing-grounds/test.py <code>
[Error - 7:03:29 p.m.] reformatted /home/ichard26/programming/webdev/blog/testing-grounds/test.py

All done! ✨ 🍰 ✨
1 file reformatted.

@ichard26
Copy link
Collaborator

pylint-dev/pylint#5796 has some good discussion related to this.

@zsol
Copy link
Collaborator

zsol commented Feb 18, 2022

I'd be happy to retire #2512 in favor of a PR that pulls in most of the backend code from that extension. I think it makes sense for Black to (optionally, just like blackd) provide a black-language-server binary that speaks LSP, and in the future we could maybe retire blackd (pending a discussion on complexity - the blackd API is very simple compared to even the fraction of LSP that will be required for black-language-server).

Owning the vscode extension is out of scope for the Black project IMO, just like we don't own other IDE integrations in general (vim being the only exception which I still disagree with).

Some notes about that backend code (but again, happy to do this on an actual PR):

  • I like the support for notebook cells
  • Not sure what that is_python check is for
  • If the code is part of Black, it won't need the _format_using_path codepath, and _format_using_module could use lower level APIs than just calling black.__main__ (like blackd)
  • Curious why there's a check for stdlib paths. Seems a bit out of place - I wonder if there's a better way to avoid accidentally reformatting files that are not part of the opened project.

@karthiknadig
Copy link
Author

@ichard26 Our plan is to breakout formatting, linting, tooling into their own extensions. We want to make it easier for the python community to contribute new tools. Currently, most of the code in the python extension is in TS, and the amount of code needed to add a new tool support (say a new formatter) is quite large. The black extension is built out of a template, where the TS code is generic, and you probably won't have to touch. So far, I have used the same TS code, in five separate extensions. The TS code's purpose is to get the correct python path from the python extension and launch format_server.py. The bulk of the LSP code exists in two python files format_server.py, and utils.py. We use pygls library as the LSP implementation in python.

Will the production build of the extension bundle Black?

Yes, and we plan on adding GH Action to create builds with the latest version of black for publishing. But the way it is designed now, the bundled code is fallback. The idea is that we will use the black version that is available in the user's environment first. There is some code already in there to handle different versions. But, in the prototype it only works with latest black.

What APIs will it require?

Currently, I am calling runpy.run_module with following arguments black --stdin-filename /home/ichard26/programming/webdev/blog/testing-grounds/test.py. This is what you see in the logs. This seems to be working well. API wise that means that we need a function that can accept, code and file_name, and can read any settings just like black running from command line and return the formatted string. Ideally, it would be great if we could control the line endings. LSP supports normalizing line endings, and it can be a pain to work around.

@zsol Thanks for the feedback. is_python check is for notebook cells. Over LSP there are two modes, whole file and cell. In cell mode, we get all the cells as separate text documents. There is no language indicator in that mode. So, the is_python check is there to avoid formatting those cells. This code in format_server.py actually supposed to be generic, we built linter support and formatting support using this same base code. That check is there in case the tool we are wrapping doesn't handle that. We can remove that for black.

@brettcannon
Copy link
Member

  • Is the team planning to publish the LSP and extension as a single project or separately?

I'm going to answer this question with a question 😁: would you find it beneficial long-term if we split it out? I was thinking we would either do it eventually or do it lazily based on community request since if non one is going to use the PyPI project for the LSP server it just becomes overhead for us. Out of all the projects we have spoken to about their respective extensions, you're the first to ask this, so if you want it I'm happy to trial it with your extension, especially if folks here are up for helping out a bit with maintaining it (and that's not asking for co-maintainership, just willing to help with a PR every so often).

@thelastWallE
Copy link

Hi. I just tried this extension. Maybe i need some other config for it to run?
I get:

Formatter Name: Black
Formatter Module: black
[ERROR 2022-3-4 9:33:5.928]: Error initializing python:  [TypeError: n.environment.onDidActiveInterpreterChanged is not a function
	at c (c:\Users\Sven\.vscode\extensions\kanadig.python-black-0.0.1\dist\extension.js:1:72961)
	at Immediate._onImmediate (c:\Users\Sven\.vscode\extensions\kanadig.python-black-0.0.1\dist\extension.js:1:301465)
	at processImmediate (node:internal/timers:464:21)]

@brettcannon
Copy link
Member

@thelastWallE probably a bigger chance something changed in VS Code since that code was written or it's just a bug. We are hoping to get a preview release of a Black extension out soon, so you will be able to try it from the VS Code Marketplace at that point.

@ichard26 ichard26 added the S: needs discussion Needs further hashing out before ready for implementation (on desirability, feasibility, etc.) label Apr 9, 2022
@ichard26
Copy link
Collaborator

ichard26 commented Apr 9, 2022

Sorry for taking a long time to get back to you!

Our plan is to breakout formatting, linting, tooling into their own extensions. We want to make it easier for the python community to contribute new tools.

Makes sense, thanks for elaborating!

Currently, I am calling runpy.run_module with following arguments black --stdin-filename /home/ichard26/programming/webdev/blog/testing-grounds/test.py. This is what you see in the logs.

That's so clever, and I love it :)

... would you find it beneficial long-term if we split it out?

For the sake of moving things along, I vote to keeping the LSP and extension as one project. If the community asks for it later on we can reconsider.


I've sent a message to the rest of the core team so hopefully we can soon share a conclusive answer!

@karthiknadig
Copy link
Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: integrations Editor plugins and other integrations C: maintenance Related to project maintenance, e.g. CI, testing, policy changes, releases S: needs discussion Needs further hashing out before ready for implementation (on desirability, feasibility, etc.) T: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants