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

Support for formatting tool "black" #1153

Closed
ocavue opened this issue Mar 22, 2018 · 25 comments · Fixed by #1611
Closed

Support for formatting tool "black" #1153

ocavue opened this issue Mar 22, 2018 · 25 comments · Fixed by #1611
Labels
area-formatting feature-request Request for new features or functionality
Milestone

Comments

@ocavue
Copy link

ocavue commented Mar 22, 2018

This issue is an enhancement request

black is a formatting tool, which has 1200 stars since 8 days ago when the first commit was pushed. It will not be long before black become a popular tool like yapf and autopep8. I think that will be great if vscode-python supports it.

@brettcannon
Copy link
Member

Due note that https://marketplace.visualstudio.com/items?itemName=joslarson.black-vscode already exists for VS Code. But we will consider adding direct integration if Black's popularity continues to grow as it currently is.

@brettcannon brettcannon added feature-request Request for new features or functionality area-formatting needs decision labels Mar 22, 2018
@Kos
Copy link

Kos commented Mar 23, 2018

I'm with OP here - Black's quick growth is extremely promising.

Moreover, since this extension already supports choosing a formatter (autopep8 / yapf), I'd much rather see Black as an alternative directly here than as a secondary extension, following the 'batteries included' spirit of this extension and VSCode in general.

@Kos
Copy link

Kos commented Mar 23, 2018

I had a look at the code, looks like the major obstacle is that Black defaults to editing the file in-place and has (currently) no support for emitting diffs (like autopep8 or yapf). It does allow formatting stdin to stdout, but I understand that's less optimal because it makes it impractical to format a range and inconvenient if formatting takes a long time? (A diff can usually be applied even if the user did some edits after formatting was triggered).

So the options look like either:
a. implement formatting with current CLI in a sub optimal way (whole file at once without diffs) and see what happens,
b. implement --diff and --lines in black, or on top of black, and then resume

Thoughts?

@ivlevdenis
Copy link

@brettcannon black is better and more beautiful formats :)

@brettcannon
Copy link
Member

Please leave a 👍 on the first comment for this issue to vote for this feature to help us prioritize our future work. Individual comments about support won't count as we sort on reactions when considering our work log.

@brettcannon
Copy link
Member

After discussing this we have agreed that we would happily look at anyone's PR to implement this, but at the moment we have other stuff taking up our time so we can't do this ourselves.

@jarshwah
Copy link

jarshwah commented Mar 29, 2018

I began a change about a week ago seeing what would be necessary. I ran into the lack of --diff option as well. Further, I kept crashing the plugin because I have no idea what I'm doing.

I've opened a PR just incase someone wants to pick up where I left off, as I'm not going to have time in the next few weeks.

#1234

I would recommend that someone also reaches out to the black project to implement diffing, as both pep8 and yapf support.

@ambv
Copy link

ambv commented Mar 31, 2018

@Kos, why do you need --lines, too?

@Kos
Copy link

Kos commented Mar 31, 2018

@ambv Thanks for looking into this! My preference is to format on save, but IIRC tools rely on --lines to implement "format selection" command. But this leaves me thinking, if formatting is fast, you could emulate --lines by applying only overlapping hunks from a patch.

@ambv
Copy link

ambv commented Mar 31, 2018

Black doesn't support only formatting of a line range because in general this might land in the middle of an expression or multiline block and we'd have to hack the parser or input to support it. I'd prefer for simplicity's sake if we avoid this.

I will make Black faster to offset this. Everybody wins.

@joslarson
Copy link

joslarson commented Apr 2, 2018

@brettcannon @Kos I'm the creator of the joslarson.black-vscode extension, and am expecting to retire it once support lands in the official python extension. I just wanted to do my part to support black in the mean time.

@joslarson
Copy link

@Kos The current black extension (joslarson.black-vscode) already supports "format selection" without a --lines option. It just pipes the selected text into the black CLI.

@ambv
Copy link

ambv commented Apr 2, 2018

@joslarson Thanks, Joseph, for your plugin. It really helped with early adoption.

Re: piping random parts of a file - as I said above, this will not work in general. Take this code:

d = {
  'a': 1,
}

Select the middle line and try to pipe it to Black. Fail.

@joslarson
Copy link

@ambv I just meant that in my extension if you select an independently valid block of python code and run "format selection" it will format just that part of the file. But if a user tries to format something that by itself is invalid, it shows them a notification "Failed to format: unable to parse input." What I'm gleaning here is that other formatters can format a line in context of the file, is that right?

@ambv
Copy link

ambv commented Apr 2, 2018

Yes, some formatters can apply selective formatting. I don't want to do that with Black because that is strictly against PEP 8 which above all else suggests staying consistent with the style found in the rest of the file. And it would be painful to implement.

@brettcannon
Copy link
Member

FYI, @ambv implemented diff output for Black in 18.4a0 so adding support should be simpler now.

And if no one gets to this prior to PyCon US I will probably do it myself just so I can get a t-shirt. 😄

@DonJayamanne
Copy link

DonJayamanne commented Apr 8, 2018

FYI - A PR is in progress #1234

@qubitron @brettcannon

Problem:

  • Users will not be able to install this tool when using a Python 2 environment.

We'll need a simple way for user to:

  1. Use the this new formatter from within VS Code in a Python 2 environment
  2. Or warn the users and provide instructions on how they can use it

Solutions:

  • When the user attempts to install this tool via our extension we could display a message
    and provide a link for manual steps
  • We could automate all of this,
    • Asking the user to select a Python 3.6.0 interpreter from a list (we have this list)
    • Install black formatter in there
    • Update the global (user) settings file (settings.json) to point to the black formatter executable

@qubitron
Copy link

qubitron commented Apr 9, 2018

@DonJayamanne I prefer to provide a warning with manual steps, all of our UI works off of the currently selected environment so would be odd to introduce a new design pattern for this. Presumably if a python 2 environment is selected the user would want it to work in Python 2 anyways.

Any reason install can't work in a Python 2 environment?

@brettcannon
Copy link
Member

@qubitron because Black only works with Python 3.6 and newer since it's a new project and who wants to support Python 2 if you don't have to? 😉

@ambv
Copy link

ambv commented Apr 9, 2018

@qubitron Black can format Python 2 code but is itself written in Python 3. Similarly to how Mypy, the type checker, can type check Python 2 code but is itself written in Python 3.

@brettcannon
Copy link
Member

So as long as we support people setting the path to Black then that will cover folks wanting to use Black on Python 2 code.

@DonJayamanne
Copy link

DonJayamanne commented Apr 9, 2018

Ok, then no changes to be made to the workflow.

@jarshwah
Copy link

Well that makes the patch much easier to complete, thanks! If there's an opportunity to provide a nicer error at time of use on py2, I'll look at doing so.

@DonJayamanne
Copy link

If there's an opportunity to provide a nicer error at time of use on py2, I'll look at doing so.

Lets leave that out of your PR, at least for now.

@brettcannon
Copy link
Member

This just got merged, so once CI passes people can test this through the development build of the extension.

@lock lock bot locked as resolved and limited conversation to collaborators Jul 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-formatting feature-request Request for new features or functionality
Projects
None yet
10 participants