psf / black

The uncompromising Python code formatter
https://black.readthedocs.io/en/stable/
MIT License
38.31k stars 2.42k forks source link

Black over Language Server Protocol and VS Code extension #2883

Open karthiknadig opened 2 years ago

karthiknadig commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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:

[...] 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:

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 commented 2 years ago

https://github.com/PyCQA/pylint/issues/5796 has some good discussion related to this.

zsol commented 2 years ago

I'd be happy to retire https://github.com/psf/black/pull/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):

karthiknadig commented 2 years ago

@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 commented 2 years ago
  • 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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

We have the extension available as pre-release: https://marketplace.visualstudio.com/items?itemName=ms-python.black-formatter

repository: https://github.com/microsoft/vscode-black-formatter Everything LSP related and black related happens here: https://github.com/microsoft/vscode-black-formatter/blob/main/bundled/formatter/format_server.py