-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[WIP] Added black as a formatter #1234
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
autopep8 | ||
yapf | ||
black | ||
pylint | ||
pep8 | ||
prospector | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// Licensed under the MIT License. | ||
|
||
'use strict'; | ||
|
||
import * as vscode from 'vscode'; | ||
import { StopWatch } from '../common/stopWatch'; | ||
import { IConfigurationService, Product } from '../common/types'; | ||
import { IServiceContainer } from '../ioc/types'; | ||
import { sendTelemetryWhenDone } from '../telemetry'; | ||
import { FORMAT } from '../telemetry/constants'; | ||
import { BaseFormatter } from './baseFormatter'; | ||
|
||
export class BlackFormatter extends BaseFormatter { | ||
constructor(serviceContainer: IServiceContainer) { | ||
super('black', Product.black, serviceContainer); | ||
} | ||
|
||
public formatDocument(document: vscode.TextDocument, options: vscode.FormattingOptions, token: vscode.CancellationToken, range?: vscode.Range): Thenable<vscode.TextEdit[]> { | ||
const stopWatch = new StopWatch(); | ||
const settings = this.serviceContainer.get<IConfigurationService>(IConfigurationService).getSettings(document.uri); | ||
const hasCustomArgs = Array.isArray(settings.formatting.blackArgs) && settings.formatting.blackArgs.length > 0; | ||
const formatSelection = false; // range ? !range.isEmpty : false; | ||
|
||
const args = ['--diff']; | ||
// if (formatSelection) { | ||
// black does not support partial formatting, throw an error? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unsure how we should handle the lack of partial formatting. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is possible, all you need to do is, send in the source of the selected content. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI, @ambv has said he purposefully doesn't support partial formatting in Black so we might want to simply not support it in this instance. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It wasn't possible to use stdin with the current way formatters are structured, without introducing some new concepts. I'm going to ignore partial formatting for now - maybe someone can attempt it in a future patch. |
||
// // tslint:disable-next-line:no-non-null-assertion | ||
// args.push(...['--lines', `${range!.start.line + 1}-${range!.end.line + 1}`]); | ||
// } | ||
// Yapf starts looking for config file starting from the file path. | ||
const fallbackFolder = this.getWorkspaceUri(document).fsPath; | ||
const cwd = this.getDocumentPath(document, fallbackFolder); | ||
const promise = super.provideDocumentFormattingEdits(document, options, token, args, cwd); | ||
//sendTelemetryWhenDone(FORMAT, promise, stopWatch, { tool: 'black', hasCustomArgs, formatSelection }); | ||
return promise; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Black outputs
Reformatted <filename>
to stderr for each file, which ends up breaking due to this. I think it'd be better to check the return code rather than inferring an error due to output on stderr - which is frequently used to communicate state rather than an error condition.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Though I don't remember why I did this. For some reason it was necessary. Please try testing it temporarily and test without the formatter installed. Ideally that should fail. If that works, my suggesting is to pass this as a flag to the base linter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm reluctant to change existing code, as I have a very vague memory of the fact that we couldn't rely on exit codes (at least for the linters).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I'll test with each of the formatters - both installed and not installed and see what happens.