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

[WIP] Added black as a formatter #1234

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -1515,7 +1515,8 @@
"enum": [
"autopep8",
"yapf",
"none"
"none",
"black"
],
"scope": "resource"
},
Expand All @@ -1531,6 +1532,12 @@
"description": "Path to yapf, you can use a custom version of yapf by modifying this setting to include the full path.",
"scope": "resource"
},
"python.formatting.blackPath": {
"type": "string",
"default": "black",
"description": "Path to black, you can use a custom version of black by modifying this setting to include the full path.",
"scope": "resource"
},
"python.formatting.autopep8Args": {
"type": "array",
"description": "Arguments passed in. Each argument is a separate item in the array.",
Expand All @@ -1549,6 +1556,15 @@
},
"scope": "resource"
},
"python.formatting.blackArgs": {
"type": "array",
"description": "Arguments passed in. Each argument is a separate item in the array.",
"default": [],
"items": {
"type": "string"
},
"scope": "resource"
},
"python.autoComplete.preloadModules": {
"type": "array",
"items": {
Expand Down
1 change: 1 addition & 0 deletions requirements.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
autopep8
yapf
black
pylint
pep8
prospector
Expand Down
4 changes: 3 additions & 1 deletion src/client/common/configSettings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -213,10 +213,12 @@ export class PythonSettings extends EventEmitter implements IPythonSettings {
this.formatting = this.formatting ? this.formatting : {
autopep8Args: [], autopep8Path: 'autopep8',
provider: 'autopep8',
yapfArgs: [], yapfPath: 'yapf'
yapfArgs: [], yapfPath: 'yapf',
blackArgs: [], blackPath: 'black'
};
this.formatting.autopep8Path = getAbsolutePath(systemVariables.resolveAny(this.formatting.autopep8Path), workspaceRoot);
this.formatting.yapfPath = getAbsolutePath(systemVariables.resolveAny(this.formatting.yapfPath), workspaceRoot);
this.formatting.blackPath = getAbsolutePath(systemVariables.resolveAny(this.formatting.blackPath), workspaceRoot);

// tslint:disable-next-line:no-backbone-get-set-outside-model no-non-null-assertion
const autoCompleteSettings = systemVariables.resolveAny(pythonSettings.get<IAutoCompeteSettings>('autoComplete'))!;
Expand Down
2 changes: 2 additions & 0 deletions src/client/common/installer/productInstaller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ class FormatterInstaller extends BaseInstaller {
const productName = ProductNames.get(product)!;

const installThis = `Install ${productName}`;
// TODO(Josh): Wat
const alternateFormatter = product === Product.autopep8 ? 'yapf' : 'autopep8';
const useOtherFormatter = `Use '${alternateFormatter}' formatter`;
const item = await this.appShell.showErrorMessage(`Formatter ${productName} is not installed.`, installThis, useOtherFormatter);
Expand Down Expand Up @@ -231,6 +232,7 @@ export class ProductInstaller implements IInstaller {
this.ProductTypes.set(Product.unittest, ProductType.TestFramework);
this.ProductTypes.set(Product.autopep8, ProductType.Formatter);
this.ProductTypes.set(Product.yapf, ProductType.Formatter);
this.ProductTypes.set(Product.black, ProductType.Formatter);
this.ProductTypes.set(Product.rope, ProductType.RefactoringLibrary);
}

Expand Down
1 change: 1 addition & 0 deletions src/client/common/installer/productNames.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,5 @@ ProductNames.set(Product.pydocstyle, 'pydocstyle');
ProductNames.set(Product.pylint, 'pylint');
ProductNames.set(Product.pytest, 'pytest');
ProductNames.set(Product.yapf, 'yapf');
ProductNames.set(Product.black, 'black');
ProductNames.set(Product.rope, 'rope');
5 changes: 4 additions & 1 deletion src/client/common/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,8 @@ export enum Product {
unittest = 12,
ctags = 13,
rope = 14,
isort = 15
isort = 15,
black = 16
}

export enum ModuleNamePurpose {
Expand Down Expand Up @@ -193,6 +194,8 @@ export interface IFormattingSettings {
readonly autopep8Args: string[];
yapfPath: string;
readonly yapfArgs: string[];
blackPath: string;
readonly blackArgs: string[];
}
export interface IAutoCompeteSettings {
readonly addBrackets: boolean;
Expand Down
2 changes: 1 addition & 1 deletion src/client/formatters/baseFormatter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export abstract class BaseFormatter {
const executionInfo = this.helper.getExecutionInfo(this.product, args, document.uri);
executionInfo.args.push(tempFile);
const pythonToolsExecutionService = this.serviceContainer.get<IPythonToolExecutionService>(IPythonToolExecutionService);
const promise = pythonToolsExecutionService.exec(executionInfo, { cwd, throwOnStdErr: true, token }, document.uri)
const promise = pythonToolsExecutionService.exec(executionInfo, { cwd, throwOnStdErr: false, token }, document.uri)
Copy link
Author

@jarshwah jarshwah Apr 7, 2018

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.

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.

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).

Copy link
Author

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.

.then(output => output.stdout)
.then(data => {
if (this.checkCancellation(document.fileName, tempFile, token)) {
Expand Down
38 changes: 38 additions & 0 deletions src/client/formatters/blackFormatter.ts
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?
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure how we should handle the lack of partial formatting.

Choose a reason for hiding this comment

The 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.
Check the command line docs (black [OPTIONS] [SRC]...) - pass in the text
https://github.com/ambv/black#command-line-options

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

The 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;
}
}
1 change: 1 addition & 0 deletions src/client/formatters/helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export class FormatterHelper implements IFormatterHelper {
switch (formatter) {
case Product.autopep8: return 'autopep8';
case Product.yapf: return 'yapf';
case Product.black: return 'black';
default: {
throw new Error(`Unrecognized Formatter '${formatter}'`);
}
Expand Down
2 changes: 1 addition & 1 deletion src/client/formatters/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { ExecutionInfo, IFormattingSettings, Product } from '../common/types';

export const IFormatterHelper = Symbol('IFormatterHelper');

export type FormatterId = 'autopep8' | 'yapf';
export type FormatterId = 'autopep8' | 'yapf' | 'black';

export type FormatterSettingsPropertyNames = {
argsName: keyof IFormattingSettings;
Expand Down
5 changes: 4 additions & 1 deletion src/client/providers/formatProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { IConfigurationService } from '../common/types';
import { IServiceContainer } from '../ioc/types';
import { AutoPep8Formatter } from './../formatters/autoPep8Formatter';
import { BaseFormatter } from './../formatters/baseFormatter';
import { BlackFormatter } from './../formatters/blackFormatter';
import { DummyFormatter } from './../formatters/dummyFormatter';
import { YapfFormatter } from './../formatters/yapfFormatter';

Expand All @@ -27,16 +28,18 @@ export class PythonFormattingEditProvider implements vscode.DocumentFormattingEd
public constructor(context: vscode.ExtensionContext, serviceContainer: IServiceContainer) {
const yapfFormatter = new YapfFormatter(serviceContainer);
const autoPep8 = new AutoPep8Formatter(serviceContainer);
const blackFormatter = new BlackFormatter(serviceContainer);
const dummy = new DummyFormatter(serviceContainer);
this.formatters.set(yapfFormatter.Id, yapfFormatter);
this.formatters.set(autoPep8.Id, autoPep8);
this.formatters.set(blackFormatter.Id, blackFormatter);
this.formatters.set(dummy.Id, dummy);

this.commands = serviceContainer.get<ICommandManager>(ICommandManager);
this.workspace = serviceContainer.get<IWorkspaceService>(IWorkspaceService);
this.documentManager = serviceContainer.get<IDocumentManager>(IDocumentManager);
this.config = serviceContainer.get<IConfigurationService>(IConfigurationService);
this.disposables.push(this.documentManager.onDidSaveTextDocument(async document => await this.onSaveDocument(document)));
this.disposables.push(this.documentManager.onDidSaveTextDocument(async document => this.onSaveDocument(document)));
}

public dispose() {
Expand Down
2 changes: 1 addition & 1 deletion src/client/telemetry/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ export type EditorLoadTelemetry = {
condaVersion: string;
};
export type FormatTelemetry = {
tool: 'autopep8' | 'yapf';
tool: 'autopep8' | 'yapf' | 'black';
hasCustomArgs: boolean;
formatSelection: boolean;
};
Expand Down
Loading