From fe89889375905b4592208350c53748b65643c77e Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Tue, 10 Oct 2023 19:14:55 -0700 Subject: [PATCH 1/3] Remove formatting support --- .../common/installer/moduleInstaller.ts | 6 - src/client/common/installer/productNames.ts | 3 - src/client/common/installer/productPath.ts | 14 -- src/client/common/installer/productService.ts | 3 - .../common/installer/serviceRegistry.ts | 6 - src/client/common/types.ts | 5 - src/client/extensionActivation.ts | 19 +- src/client/formatters/autoPep8Formatter.ts | 43 ---- src/client/formatters/baseFormatter.ts | 149 ------------- src/client/formatters/blackFormatter.ts | 55 ----- src/client/formatters/dummyFormatter.ts | 19 -- src/client/formatters/helper.ts | 53 ----- src/client/formatters/serviceRegistry.ts | 10 - src/client/formatters/types.ts | 20 -- src/client/formatters/yapfFormatter.ts | 38 ---- src/client/providers/formatProvider.ts | 135 ------------ src/test/common/installer.test.ts | 12 +- .../installer/channelManager.unit.test.ts | 6 +- .../common/installer/productPath.unit.test.ts | 45 +--- .../installer/serviceRegistry.unit.test.ts | 8 - src/test/common/moduleInstaller.test.ts | 1 - src/test/format/extension.format.test.ts | 205 ------------------ src/test/format/format.helper.test.ts | 117 ---------- src/test/format/formatter.unit.test.ts | 171 --------------- src/test/linters/lint.multiroot.test.ts | 11 +- src/test/linters/lint.test.ts | 11 +- src/test/serviceRegistry.ts | 5 - 27 files changed, 9 insertions(+), 1161 deletions(-) delete mode 100644 src/client/formatters/autoPep8Formatter.ts delete mode 100644 src/client/formatters/baseFormatter.ts delete mode 100644 src/client/formatters/blackFormatter.ts delete mode 100644 src/client/formatters/dummyFormatter.ts delete mode 100644 src/client/formatters/helper.ts delete mode 100644 src/client/formatters/serviceRegistry.ts delete mode 100644 src/client/formatters/types.ts delete mode 100644 src/client/formatters/yapfFormatter.ts delete mode 100644 src/client/providers/formatProvider.ts delete mode 100644 src/test/format/extension.format.test.ts delete mode 100644 src/test/format/format.helper.test.ts delete mode 100644 src/test/format/formatter.unit.test.ts diff --git a/src/client/common/installer/moduleInstaller.ts b/src/client/common/installer/moduleInstaller.ts index f70dd937aba9..5a4f245900ea 100644 --- a/src/client/common/installer/moduleInstaller.ts +++ b/src/client/common/installer/moduleInstaller.ts @@ -248,16 +248,10 @@ export function translateProductToModule(product: Product): string { return 'pylint'; case Product.pytest: return 'pytest'; - case Product.autopep8: - return 'autopep8'; - case Product.black: - return 'black'; case Product.pycodestyle: return 'pycodestyle'; case Product.pydocstyle: return 'pydocstyle'; - case Product.yapf: - return 'yapf'; case Product.flake8: return 'flake8'; case Product.unittest: diff --git a/src/client/common/installer/productNames.ts b/src/client/common/installer/productNames.ts index 378fd5a38dba..9b917d2f1d76 100644 --- a/src/client/common/installer/productNames.ts +++ b/src/client/common/installer/productNames.ts @@ -4,9 +4,7 @@ import { Product } from '../types'; export const ProductNames = new Map(); -ProductNames.set(Product.autopep8, 'autopep8'); ProductNames.set(Product.bandit, 'bandit'); -ProductNames.set(Product.black, 'black'); ProductNames.set(Product.flake8, 'flake8'); ProductNames.set(Product.mypy, 'mypy'); ProductNames.set(Product.pycodestyle, 'pycodestyle'); @@ -15,7 +13,6 @@ ProductNames.set(Product.prospector, 'prospector'); ProductNames.set(Product.pydocstyle, 'pydocstyle'); ProductNames.set(Product.pylint, 'pylint'); ProductNames.set(Product.pytest, 'pytest'); -ProductNames.set(Product.yapf, 'yapf'); ProductNames.set(Product.tensorboard, 'tensorboard'); ProductNames.set(Product.torchProfilerInstallName, 'torch-tb-profiler'); ProductNames.set(Product.torchProfilerImportName, 'torch_tb_profiler'); diff --git a/src/client/common/installer/productPath.ts b/src/client/common/installer/productPath.ts index 5c36a6bbd3bd..3b3f1d7c1794 100644 --- a/src/client/common/installer/productPath.ts +++ b/src/client/common/installer/productPath.ts @@ -6,7 +6,6 @@ import { inject, injectable } from 'inversify'; import * as path from 'path'; import { Uri } from 'vscode'; -import { IFormatterHelper } from '../../formatters/types'; import { IServiceContainer } from '../../ioc/types'; import { ILinterManager } from '../../linters/types'; import { ITestingService } from '../../testing/types'; @@ -37,19 +36,6 @@ export abstract class BaseProductPathsService implements IProductPathService { } } -@injectable() -export class FormatterProductPathService extends BaseProductPathsService { - constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { - super(serviceContainer); - } - public getExecutableNameFromSettings(product: Product, resource?: Uri): string { - const settings = this.configService.getSettings(resource); - const formatHelper = this.serviceContainer.get(IFormatterHelper); - const settingsPropNames = formatHelper.getSettingsPropertyNames(product); - return settings.formatting[settingsPropNames.pathName] as string; - } -} - @injectable() export class LinterProductPathService extends BaseProductPathsService { constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { diff --git a/src/client/common/installer/productService.ts b/src/client/common/installer/productService.ts index 26a01e37c3ba..af2192755fe8 100644 --- a/src/client/common/installer/productService.ts +++ b/src/client/common/installer/productService.ts @@ -22,9 +22,6 @@ export class ProductService implements IProductService { this.ProductTypes.set(Product.pylint, ProductType.Linter); this.ProductTypes.set(Product.pytest, ProductType.TestFramework); this.ProductTypes.set(Product.unittest, ProductType.TestFramework); - this.ProductTypes.set(Product.autopep8, ProductType.Formatter); - this.ProductTypes.set(Product.black, ProductType.Formatter); - this.ProductTypes.set(Product.yapf, ProductType.Formatter); this.ProductTypes.set(Product.tensorboard, ProductType.DataScience); this.ProductTypes.set(Product.torchProfilerInstallName, ProductType.DataScience); this.ProductTypes.set(Product.torchProfilerImportName, ProductType.DataScience); diff --git a/src/client/common/installer/serviceRegistry.ts b/src/client/common/installer/serviceRegistry.ts index c262c7571711..c4e7c1a089c6 100644 --- a/src/client/common/installer/serviceRegistry.ts +++ b/src/client/common/installer/serviceRegistry.ts @@ -11,7 +11,6 @@ import { PipInstaller } from './pipInstaller'; import { PoetryInstaller } from './poetryInstaller'; import { DataScienceProductPathService, - FormatterProductPathService, LinterProductPathService, TestFrameworkProductPathService, } from './productPath'; @@ -25,11 +24,6 @@ export function registerTypes(serviceManager: IServiceManager) { serviceManager.addSingleton(IModuleInstaller, PoetryInstaller); serviceManager.addSingleton(IInstallationChannelManager, InstallationChannelManager); serviceManager.addSingleton(IProductService, ProductService); - serviceManager.addSingleton( - IProductPathService, - FormatterProductPathService, - ProductType.Formatter, - ); serviceManager.addSingleton(IProductPathService, LinterProductPathService, ProductType.Linter); serviceManager.addSingleton( IProductPathService, diff --git a/src/client/common/types.ts b/src/client/common/types.ts index 07f1fea6b86b..8b90443703c6 100644 --- a/src/client/common/types.ts +++ b/src/client/common/types.ts @@ -87,7 +87,6 @@ export enum ProductInstallStatus { export enum ProductType { Linter = 'Linter', - Formatter = 'Formatter', TestFramework = 'TestFramework', RefactoringLibrary = 'RefactoringLibrary', DataScience = 'DataScience', @@ -102,11 +101,8 @@ export enum Product { pylama = 6, prospector = 7, pydocstyle = 8, - yapf = 9, - autopep8 = 10, mypy = 11, unittest = 12, - black = 16, bandit = 17, tensorboard = 24, torchProfilerInstallName = 25, @@ -185,7 +181,6 @@ export interface IPythonSettings { readonly poetryPath: string; readonly devOptions: string[]; readonly linting: ILintingSettings; - readonly formatting: IFormattingSettings; readonly testing: ITestingSettings; readonly autoComplete: IAutoCompleteSettings; readonly terminal: ITerminalSettings; diff --git a/src/client/extensionActivation.ts b/src/client/extensionActivation.ts index 807698f3ec29..7730427878f7 100644 --- a/src/client/extensionActivation.ts +++ b/src/client/extensionActivation.ts @@ -10,7 +10,7 @@ import { IExtensionActivationManager } from './activation/types'; import { registerTypes as appRegisterTypes } from './application/serviceRegistry'; import { IApplicationDiagnostics } from './application/types'; import { IApplicationEnvironment, ICommandManager, IWorkspaceService } from './common/application/types'; -import { Commands, PYTHON, PYTHON_LANGUAGE, UseProposedApi } from './common/constants'; +import { Commands, PYTHON_LANGUAGE, UseProposedApi } from './common/constants'; import { registerTypes as installerRegisterTypes } from './common/installer/serviceRegistry'; import { IFileSystem } from './common/platform/types'; import { @@ -25,11 +25,9 @@ import { noop } from './common/utils/misc'; import { DebuggerTypeName } from './debugger/constants'; import { registerTypes as debugConfigurationRegisterTypes } from './debugger/extension/serviceRegistry'; import { IDebugConfigurationService, IDynamicDebugConfigurationService } from './debugger/extension/types'; -import { registerTypes as formattersRegisterTypes } from './formatters/serviceRegistry'; import { IInterpreterService } from './interpreter/contracts'; import { getLanguageConfiguration } from './language/languageConfiguration'; import { registerTypes as lintersRegisterTypes } from './linters/serviceRegistry'; -import { PythonFormattingEditProvider } from './providers/formatProvider'; import { ReplProvider } from './providers/replProvider'; import { registerTypes as providersRegisterTypes } from './providers/serviceRegistry'; import { TerminalProvider } from './providers/terminalProvider'; @@ -110,7 +108,7 @@ export function activateFeatures(ext: ExtensionState, _components: Components): // See https://github.com/microsoft/vscode-python/issues/10454. async function activateLegacy(ext: ExtensionState): Promise { - const { context, legacyIOC } = ext; + const { legacyIOC } = ext; const { serviceManager, serviceContainer } = legacyIOC; // register "services" @@ -125,7 +123,6 @@ async function activateLegacy(ext: ExtensionState): Promise { // Feature specific registrations. unitTestsRegisterTypes(serviceManager); lintersRegisterTypes(serviceManager); - formattersRegisterTypes(serviceManager); installerRegisterTypes(serviceManager); commonRegisterTerminalTypes(serviceManager); debugConfigurationRegisterTypes(serviceManager); @@ -134,7 +131,6 @@ async function activateLegacy(ext: ExtensionState): Promise { const extensions = serviceContainer.get(IExtensions); await setDefaultLanguageServer(extensions, serviceManager); - const configuration = serviceManager.get(IConfigurationService); // Settings are dependent on Experiment service, so we need to initialize it after experiments are activated. serviceContainer.get(IConfigurationService).getSettings().register(); @@ -165,20 +161,9 @@ async function activateLegacy(ext: ExtensionState): Promise { serviceContainer.get(IApplicationDiagnostics).register(); serviceManager.get(ITerminalAutoActivation).register(); - const pythonSettings = configuration.getSettings(); serviceManager.get(ICodeExecutionManager).registerCommands(); - if ( - pythonSettings && - pythonSettings.formatting && - pythonSettings.formatting.provider !== 'internalConsole' - ) { - const formatProvider = new PythonFormattingEditProvider(context, serviceContainer); - disposables.push(languages.registerDocumentFormattingEditProvider(PYTHON, formatProvider)); - disposables.push(languages.registerDocumentRangeFormattingEditProvider(PYTHON, formatProvider)); - } - disposables.push(new ReplProvider(serviceContainer)); const terminalProvider = new TerminalProvider(serviceContainer); diff --git a/src/client/formatters/autoPep8Formatter.ts b/src/client/formatters/autoPep8Formatter.ts deleted file mode 100644 index bf1285a60b58..000000000000 --- a/src/client/formatters/autoPep8Formatter.ts +++ /dev/null @@ -1,43 +0,0 @@ -import * as vscode from 'vscode'; -import { Product } from '../common/installer/productInstaller'; -import { IConfigurationService } from '../common/types'; -import { StopWatch } from '../common/utils/stopWatch'; -import { IServiceContainer } from '../ioc/types'; -import { sendTelemetryWhenDone } from '../telemetry'; -import { EventName } from '../telemetry/constants'; -import { BaseFormatter } from './baseFormatter'; - -export class AutoPep8Formatter extends BaseFormatter { - constructor(serviceContainer: IServiceContainer) { - super('autopep8', Product.autopep8, serviceContainer); - } - - public formatDocument( - document: vscode.TextDocument, - options: vscode.FormattingOptions, - token: vscode.CancellationToken, - range?: vscode.Range, - ): Thenable { - const stopWatch = new StopWatch(); - const settings = this.serviceContainer - .get(IConfigurationService) - .getSettings(document.uri); - const hasCustomArgs = - Array.isArray(settings.formatting.autopep8Args) && settings.formatting.autopep8Args.length > 0; - const formatSelection = range ? !range.isEmpty : false; - - const autoPep8Args = ['--diff']; - if (formatSelection) { - autoPep8Args.push( - ...['--line-range', (range!.start.line + 1).toString(), (range!.end.line + 1).toString()], - ); - } - const promise = super.provideDocumentFormattingEdits(document, options, token, autoPep8Args); - sendTelemetryWhenDone(EventName.FORMAT, promise, stopWatch, { - tool: 'autopep8', - hasCustomArgs, - formatSelection, - }); - return promise; - } -} diff --git a/src/client/formatters/baseFormatter.ts b/src/client/formatters/baseFormatter.ts deleted file mode 100644 index 64e7d15a3d45..000000000000 --- a/src/client/formatters/baseFormatter.ts +++ /dev/null @@ -1,149 +0,0 @@ -import * as path from 'path'; -import * as vscode from 'vscode'; -import { IApplicationShell, IWorkspaceService } from '../common/application/types'; -import '../common/extensions'; -import { isNotInstalledError } from '../common/helpers'; -import { IFileSystem } from '../common/platform/types'; -import { IPythonToolExecutionService } from '../common/process/types'; -import { IDisposableRegistry, IInstaller, Product } from '../common/types'; -import { isNotebookCell } from '../common/utils/misc'; -import { IServiceContainer } from '../ioc/types'; -import { traceError } from '../logging'; -import { getTempFileWithDocumentContents, getTextEditsFromPatch } from './../common/editor'; -import { IFormatterHelper } from './types'; -import { IInstallFormatterPrompt } from '../providers/prompts/types'; - -export abstract class BaseFormatter { - protected readonly workspace: IWorkspaceService; - private readonly helper: IFormatterHelper; - private errorShown: boolean = false; - - constructor(public Id: string, private product: Product, protected serviceContainer: IServiceContainer) { - this.helper = serviceContainer.get(IFormatterHelper); - this.workspace = serviceContainer.get(IWorkspaceService); - } - - public abstract formatDocument( - document: vscode.TextDocument, - options: vscode.FormattingOptions, - token: vscode.CancellationToken, - range?: vscode.Range, - ): Thenable; - protected getDocumentPath(document: vscode.TextDocument, fallbackPath: string) { - if (path.basename(document.uri.fsPath) === document.uri.fsPath) { - return fallbackPath; - } - return path.dirname(document.fileName); - } - protected getWorkspaceUri(document: vscode.TextDocument) { - const workspaceFolder = this.workspace.getWorkspaceFolder(document.uri); - if (workspaceFolder) { - return workspaceFolder.uri; - } - const folders = this.workspace.workspaceFolders; - if (Array.isArray(folders) && folders.length > 0) { - return folders[0].uri; - } - return vscode.Uri.file(__dirname); - } - protected async provideDocumentFormattingEdits( - document: vscode.TextDocument, - _options: vscode.FormattingOptions, - token: vscode.CancellationToken, - args: string[], - cwd?: string, - ): Promise { - if (typeof cwd !== 'string' || cwd.length === 0) { - cwd = this.getWorkspaceUri(document).fsPath; - } - - // autopep8 and yapf have the ability to read from the process input stream and return the formatted code out of the output stream. - // However they don't support returning the diff of the formatted text when reading data from the input stream. - // Yet getting text formatted that way avoids having to create a temporary file, however the diffing will have - // to be done here in node (extension), i.e. extension CPU, i.e. less responsive solution. - // Also, always create temp files for Notebook cells. - const tempFile = await this.createTempFile(document); - if (this.checkCancellation(document.fileName, tempFile, token)) { - return []; - } - - const executionInfo = this.helper.getExecutionInfo(this.product, args, document.uri); - executionInfo.args.push(tempFile); - const pythonToolsExecutionService = this.serviceContainer.get( - IPythonToolExecutionService, - ); - const promise = pythonToolsExecutionService - .exec(executionInfo, { cwd, throwOnStdErr: false, token }, document.uri) - .then((output) => output.stdout) - .then((data) => { - if (this.checkCancellation(document.fileName, tempFile, token)) { - return [] as vscode.TextEdit[]; - } - return getTextEditsFromPatch(document.getText(), data); - }) - .catch((error) => { - if (this.checkCancellation(document.fileName, tempFile, token)) { - return [] as vscode.TextEdit[]; - } - - this.handleError(this.Id, error, document.uri).catch(() => {}); - return [] as vscode.TextEdit[]; - }) - .then((edits) => { - this.deleteTempFile(document.fileName, tempFile).ignoreErrors(); - return edits; - }); - - const appShell = this.serviceContainer.get(IApplicationShell); - const disposableRegistry = this.serviceContainer.get(IDisposableRegistry); - const disposable = appShell.setStatusBarMessage(`Formatting with ${this.Id}`, promise); - disposableRegistry.push(disposable); - return promise; - } - - protected async handleError(_expectedFileName: string, error: Error, resource?: vscode.Uri) { - if (isNotInstalledError(error)) { - const prompt = this.serviceContainer.get(IInstallFormatterPrompt); - if (!(await prompt.showInstallFormatterPrompt(resource))) { - const installer = this.serviceContainer.get(IInstaller); - const isInstalled = await installer.isInstalled(this.product, resource); - if (!isInstalled && !this.errorShown) { - traceError( - `\nPlease install '${this.Id}' into your environment.`, - "\nIf you don't want to use it you can turn it off or use another formatter in the settings.", - ); - this.errorShown = true; - } - } - } - - traceError(`Formatting with ${this.Id} failed:\n${error}`); - } - - /** - * Always create a temporary file when formatting notebook cells. - * This is because there is no physical file associated with notebook cells (they are all virtual). - */ - private async createTempFile(document: vscode.TextDocument): Promise { - const fs = this.serviceContainer.get(IFileSystem); - return document.isDirty || isNotebookCell(document) - ? getTempFileWithDocumentContents(document, fs) - : document.fileName; - } - - private deleteTempFile(originalFile: string, tempFile: string): Promise { - if (originalFile !== tempFile) { - const fs = this.serviceContainer.get(IFileSystem); - return fs.deleteFile(tempFile); - } - return Promise.resolve(); - } - - private checkCancellation(originalFile: string, tempFile: string, token?: vscode.CancellationToken): boolean { - if (token && token.isCancellationRequested) { - this.deleteTempFile(originalFile, tempFile).ignoreErrors(); - return true; - } - return false; - } -} diff --git a/src/client/formatters/blackFormatter.ts b/src/client/formatters/blackFormatter.ts deleted file mode 100644 index 0a8109e163e0..000000000000 --- a/src/client/formatters/blackFormatter.ts +++ /dev/null @@ -1,55 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -'use strict'; - -import * as path from 'path'; -import * as vscode from 'vscode'; -import { IApplicationShell } from '../common/application/types'; -import { Product } from '../common/installer/productInstaller'; -import { IConfigurationService } from '../common/types'; -import { noop } from '../common/utils/misc'; -import { StopWatch } from '../common/utils/stopWatch'; -import { IServiceContainer } from '../ioc/types'; -import { sendTelemetryWhenDone } from '../telemetry'; -import { EventName } from '../telemetry/constants'; -import { BaseFormatter } from './baseFormatter'; - -export class BlackFormatter extends BaseFormatter { - constructor(serviceContainer: IServiceContainer) { - super('black', Product.black, serviceContainer); - } - - public async formatDocument( - document: vscode.TextDocument, - options: vscode.FormattingOptions, - token: vscode.CancellationToken, - range?: vscode.Range, - ): Promise { - const stopWatch = new StopWatch(); - const settings = this.serviceContainer - .get(IConfigurationService) - .getSettings(document.uri); - const hasCustomArgs = Array.isArray(settings.formatting.blackArgs) && settings.formatting.blackArgs.length > 0; - const formatSelection = range ? !range.isEmpty : false; - - if (formatSelection) { - const shell = this.serviceContainer.get(IApplicationShell); - // Black does not support partial formatting on purpose. - shell - .showErrorMessage(vscode.l10n.t('Black does not support the "Format Selection" command')) - .then(noop, noop); - return []; - } - - const blackArgs = ['--diff', '--quiet']; - - if (path.extname(document.fileName) === '.pyi') { - blackArgs.push('--pyi'); - } - - const promise = super.provideDocumentFormattingEdits(document, options, token, blackArgs); - sendTelemetryWhenDone(EventName.FORMAT, promise, stopWatch, { tool: 'black', hasCustomArgs, formatSelection }); - return promise; - } -} diff --git a/src/client/formatters/dummyFormatter.ts b/src/client/formatters/dummyFormatter.ts deleted file mode 100644 index b4fdba9fbc0f..000000000000 --- a/src/client/formatters/dummyFormatter.ts +++ /dev/null @@ -1,19 +0,0 @@ -import * as vscode from 'vscode'; -import { Product } from '../common/types'; -import { IServiceContainer } from '../ioc/types'; -import { BaseFormatter } from './baseFormatter'; - -export class DummyFormatter extends BaseFormatter { - constructor(serviceContainer: IServiceContainer) { - super('none', Product.yapf, serviceContainer); - } - - public formatDocument( - _document: vscode.TextDocument, - _options: vscode.FormattingOptions, - _token: vscode.CancellationToken, - _range?: vscode.Range, - ): Thenable { - return Promise.resolve([]); - } -} diff --git a/src/client/formatters/helper.ts b/src/client/formatters/helper.ts deleted file mode 100644 index ac305b51e785..000000000000 --- a/src/client/formatters/helper.ts +++ /dev/null @@ -1,53 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -import { inject, injectable } from 'inversify'; -import * as path from 'path'; -import { Uri } from 'vscode'; -import { ExecutionInfo, IConfigurationService, IFormattingSettings, Product } from '../common/types'; -import { IServiceContainer } from '../ioc/types'; -import { FormatterId, FormatterSettingsPropertyNames, IFormatterHelper } from './types'; - -@injectable() -export class FormatterHelper implements IFormatterHelper { - constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) {} - public translateToId(formatter: Product): FormatterId { - switch (formatter) { - case Product.autopep8: - return 'autopep8'; - case Product.black: - return 'black'; - case Product.yapf: - return 'yapf'; - default: { - throw new Error(`Unrecognized Formatter '${formatter}'`); - } - } - } - public getSettingsPropertyNames(formatter: Product): FormatterSettingsPropertyNames { - const id = this.translateToId(formatter); - return { - argsName: `${id}Args` as keyof IFormattingSettings, - pathName: `${id}Path` as keyof IFormattingSettings, - }; - } - public getExecutionInfo(formatter: Product, customArgs: string[], resource?: Uri): ExecutionInfo { - const settings = this.serviceContainer.get(IConfigurationService).getSettings(resource); - const names = this.getSettingsPropertyNames(formatter); - - const execPath = settings.formatting[names.pathName] as string; - let args: string[] = Array.isArray(settings.formatting[names.argsName]) - ? (settings.formatting[names.argsName] as string[]) - : []; - args = args.concat(customArgs); - - let moduleName: string | undefined; - - // If path information is not available, then treat it as a module, - if (path.basename(execPath) === execPath) { - moduleName = execPath; - } - - return { execPath, moduleName, args, product: formatter }; - } -} diff --git a/src/client/formatters/serviceRegistry.ts b/src/client/formatters/serviceRegistry.ts deleted file mode 100644 index 196e6c806b5f..000000000000 --- a/src/client/formatters/serviceRegistry.ts +++ /dev/null @@ -1,10 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -import { IServiceManager } from '../ioc/types'; -import { FormatterHelper } from './helper'; -import { IFormatterHelper } from './types'; - -export function registerTypes(serviceManager: IServiceManager) { - serviceManager.addSingleton(IFormatterHelper, FormatterHelper); -} diff --git a/src/client/formatters/types.ts b/src/client/formatters/types.ts deleted file mode 100644 index 7f4bcf5b7524..000000000000 --- a/src/client/formatters/types.ts +++ /dev/null @@ -1,20 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -import { Uri } from 'vscode'; -import { ExecutionInfo, IFormattingSettings, Product } from '../common/types'; - -export const IFormatterHelper = Symbol('IFormatterHelper'); - -export type FormatterId = 'autopep8' | 'black' | 'yapf'; - -export type FormatterSettingsPropertyNames = { - argsName: keyof IFormattingSettings; - pathName: keyof IFormattingSettings; -}; - -export interface IFormatterHelper { - translateToId(formatter: Product): FormatterId; - getSettingsPropertyNames(formatter: Product): FormatterSettingsPropertyNames; - getExecutionInfo(formatter: Product, customArgs: string[], resource?: Uri): ExecutionInfo; -} diff --git a/src/client/formatters/yapfFormatter.ts b/src/client/formatters/yapfFormatter.ts deleted file mode 100644 index 08729a97694f..000000000000 --- a/src/client/formatters/yapfFormatter.ts +++ /dev/null @@ -1,38 +0,0 @@ -import * as vscode from 'vscode'; -import { IConfigurationService, Product } from '../common/types'; -import { StopWatch } from '../common/utils/stopWatch'; -import { IServiceContainer } from '../ioc/types'; -import { sendTelemetryWhenDone } from '../telemetry'; -import { EventName } from '../telemetry/constants'; -import { BaseFormatter } from './baseFormatter'; - -export class YapfFormatter extends BaseFormatter { - constructor(serviceContainer: IServiceContainer) { - super('yapf', Product.yapf, serviceContainer); - } - - public formatDocument( - document: vscode.TextDocument, - options: vscode.FormattingOptions, - token: vscode.CancellationToken, - range?: vscode.Range, - ): Thenable { - const stopWatch = new StopWatch(); - const settings = this.serviceContainer - .get(IConfigurationService) - .getSettings(document.uri); - const hasCustomArgs = Array.isArray(settings.formatting.yapfArgs) && settings.formatting.yapfArgs.length > 0; - const formatSelection = range ? !range.isEmpty : false; - - const yapfArgs = ['--diff']; - if (formatSelection && range !== undefined) { - yapfArgs.push(...['--lines', `${range.start.line + 1}-${range.end.line + 1}`]); - } - // Yapf starts looking for config file starting from the file path. - const fallbarFolder = this.getWorkspaceUri(document).fsPath; - const cwd = this.getDocumentPath(document, fallbarFolder); - const promise = super.provideDocumentFormattingEdits(document, options, token, yapfArgs, cwd); - sendTelemetryWhenDone(EventName.FORMAT, promise, stopWatch, { tool: 'yapf', hasCustomArgs, formatSelection }); - return promise; - } -} diff --git a/src/client/providers/formatProvider.ts b/src/client/providers/formatProvider.ts deleted file mode 100644 index 1ea239c03bec..000000000000 --- a/src/client/providers/formatProvider.ts +++ /dev/null @@ -1,135 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -import * as vscode from 'vscode'; -import { ICommandManager, IDocumentManager, IWorkspaceService } from '../common/application/types'; -import { PYTHON_LANGUAGE } from '../common/constants'; -import { IConfigurationService } from '../common/types'; -import { IInterpreterService } from '../interpreter/contracts'; -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'; - -export class PythonFormattingEditProvider - implements vscode.DocumentFormattingEditProvider, vscode.DocumentRangeFormattingEditProvider, vscode.Disposable { - private readonly config: IConfigurationService; - - private readonly workspace: IWorkspaceService; - - private readonly documentManager: IDocumentManager; - - private readonly commands: ICommandManager; - - private formatters = new Map(); - - private disposables: vscode.Disposable[] = []; - - // Workaround for https://github.com/Microsoft/vscode/issues/41194 - private documentVersionBeforeFormatting = -1; - - private formatterMadeChanges = false; - - private saving = false; - - public constructor(_context: vscode.ExtensionContext, serviceContainer: IServiceContainer) { - const yapfFormatter = new YapfFormatter(serviceContainer); - const autoPep8 = new AutoPep8Formatter(serviceContainer); - const black = new BlackFormatter(serviceContainer); - const dummy = new DummyFormatter(serviceContainer); - this.formatters.set(yapfFormatter.Id, yapfFormatter); - this.formatters.set(black.Id, black); - this.formatters.set(autoPep8.Id, autoPep8); - this.formatters.set(dummy.Id, dummy); - - this.commands = serviceContainer.get(ICommandManager); - this.workspace = serviceContainer.get(IWorkspaceService); - this.documentManager = serviceContainer.get(IDocumentManager); - this.config = serviceContainer.get(IConfigurationService); - const interpreterService = serviceContainer.get(IInterpreterService); - this.disposables.push( - this.documentManager.onDidSaveTextDocument(async (document) => this.onSaveDocument(document)), - ); - this.disposables.push( - interpreterService.onDidChangeInterpreter(async () => { - if (this.documentManager.activeTextEditor) { - return this.onSaveDocument(this.documentManager.activeTextEditor.document); - } - - return undefined; - }), - ); - } - - public dispose(): void { - this.disposables.forEach((d) => d.dispose()); - } - - public provideDocumentFormattingEdits( - document: vscode.TextDocument, - options: vscode.FormattingOptions, - token: vscode.CancellationToken, - ): Promise { - return this.provideDocumentRangeFormattingEdits(document, undefined, options, token); - } - - public async provideDocumentRangeFormattingEdits( - document: vscode.TextDocument, - range: vscode.Range | undefined, - options: vscode.FormattingOptions, - token: vscode.CancellationToken, - ): Promise { - // Workaround for https://github.com/Microsoft/vscode/issues/41194 - // VSC rejects 'format on save' promise in 750 ms. Python formatting may take quite a bit longer. - // Workaround is to resolve promise to nothing here, then execute format document and force new save. - // However, we need to know if this is 'format document' or formatting on save. - - if (this.saving || document.languageId !== PYTHON_LANGUAGE) { - // We are saving after formatting (see onSaveDocument below) - // so we do not want to format again. - return []; - } - - // Remember content before formatting so we can detect if - // formatting edits have been really applied - const editorConfig = this.workspace.getConfiguration('editor', document.uri); - if (editorConfig.get('formatOnSave') === true) { - this.documentVersionBeforeFormatting = document.version; - } - - const settings = this.config.getSettings(document.uri); - const formatter = this.formatters.get(settings.formatting.provider)!; - const edits = await formatter.formatDocument(document, options, token, range); - - this.formatterMadeChanges = edits.length > 0; - return edits; - } - - private async onSaveDocument(document: vscode.TextDocument): Promise { - // Promise was rejected = formatting took too long. - // Don't format inside the event handler, do it on timeout - setTimeout(() => { - try { - if ( - this.formatterMadeChanges && - !document.isDirty && - document.version === this.documentVersionBeforeFormatting - ) { - // Formatter changes were not actually applied due to the timeout on save. - // Force formatting now and then save the document. - this.commands.executeCommand('editor.action.formatDocument').then(async () => { - this.saving = true; - await document.save(); - this.saving = false; - }); - } - } finally { - this.documentVersionBeforeFormatting = -1; - this.saving = false; - this.formatterMadeChanges = false; - } - }, 50); - } -} diff --git a/src/test/common/installer.test.ts b/src/test/common/installer.test.ts index 5c1842a2c97c..15c745cbd64f 100644 --- a/src/test/common/installer.test.ts +++ b/src/test/common/installer.test.ts @@ -28,11 +28,7 @@ import { EditorUtils } from '../../client/common/editor'; import { ExperimentService } from '../../client/common/experiments/service'; import { InstallationChannelManager } from '../../client/common/installer/channelManager'; import { ProductInstaller } from '../../client/common/installer/productInstaller'; -import { - FormatterProductPathService, - LinterProductPathService, - TestFrameworkProductPathService, -} from '../../client/common/installer/productPath'; +import { LinterProductPathService, TestFrameworkProductPathService } from '../../client/common/installer/productPath'; import { ProductService } from '../../client/common/installer/productService'; import { IInstallationChannelManager, @@ -131,7 +127,6 @@ suite('Installer', () => { ioc.registerFileSystemTypes(); ioc.registerVariableTypes(); ioc.registerLinterTypes(); - ioc.registerFormatterTypes(); ioc.registerInterpreterStorageTypes(); ioc.serviceManager.addSingleton(IPersistentStateFactory, PersistentStateFactory); @@ -159,11 +154,6 @@ suite('Installer', () => { ioc.registerMockProcessTypes(); ioc.serviceManager.addSingletonInstance(IsWindows, false); ioc.serviceManager.addSingletonInstance(IProductService, new ProductService()); - ioc.serviceManager.addSingleton( - IProductPathService, - FormatterProductPathService, - ProductType.Formatter, - ); ioc.serviceManager.addSingleton( IProductPathService, LinterProductPathService, diff --git a/src/test/common/installer/channelManager.unit.test.ts b/src/test/common/installer/channelManager.unit.test.ts index 319a9647fec7..9789f9f18718 100644 --- a/src/test/common/installer/channelManager.unit.test.ts +++ b/src/test/common/installer/channelManager.unit.test.ts @@ -57,7 +57,7 @@ suite('InstallationChannelManager - getInstallationChannel()', () => { showNoInstallersMessage.resolves(); installChannelManager = new InstallationChannelManager(serviceContainer.object); - const channel = await installChannelManager.getInstallationChannel(Product.autopep8, resource); + const channel = await installChannelManager.getInstallationChannel(Product.pytest, resource); expect(channel).to.equal(undefined, 'should be undefined'); assert.ok(showNoInstallersMessage.calledOnceWith(resource)); }); @@ -79,7 +79,7 @@ suite('InstallationChannelManager - getInstallationChannel()', () => { showNoInstallersMessage.resolves(); installChannelManager = new InstallationChannelManager(serviceContainer.object); - const channel = await installChannelManager.getInstallationChannel(Product.autopep8, resource); + const channel = await installChannelManager.getInstallationChannel(Product.pytest, resource); assert.ok(showNoInstallersMessage.notCalled); appShell.verifyAll(); expect(channel).to.equal(undefined, 'Channel should not be set'); @@ -107,7 +107,7 @@ suite('InstallationChannelManager - getInstallationChannel()', () => { showNoInstallersMessage.resolves(); installChannelManager = new InstallationChannelManager(serviceContainer.object); - const channel = await installChannelManager.getInstallationChannel(Product.autopep8, resource); + const channel = await installChannelManager.getInstallationChannel(Product.pytest, resource); assert.ok(showNoInstallersMessage.notCalled); appShell.verifyAll(); expect(channel).to.not.equal(undefined, 'Channel should be set'); diff --git a/src/test/common/installer/productPath.unit.test.ts b/src/test/common/installer/productPath.unit.test.ts index 0f627289da70..8e65f3a5caed 100644 --- a/src/test/common/installer/productPath.unit.test.ts +++ b/src/test/common/installer/productPath.unit.test.ts @@ -12,21 +12,12 @@ import '../../../client/common/extensions'; import { ProductInstaller } from '../../../client/common/installer/productInstaller'; import { BaseProductPathsService, - FormatterProductPathService, LinterProductPathService, TestFrameworkProductPathService, } from '../../../client/common/installer/productPath'; import { ProductService } from '../../../client/common/installer/productService'; import { IProductService } from '../../../client/common/installer/types'; -import { - IConfigurationService, - IFormattingSettings, - IInstaller, - IPythonSettings, - Product, - ProductType, -} from '../../../client/common/types'; -import { IFormatterHelper } from '../../../client/formatters/types'; +import { IConfigurationService, IInstaller, IPythonSettings, Product, ProductType } from '../../../client/common/types'; import { IServiceContainer } from '../../../client/ioc/types'; import { ILinterInfo, ILinterManager } from '../../../client/linters/types'; import { ITestsHelper } from '../../../client/testing/common/types'; @@ -44,7 +35,6 @@ suite('Product Path', () => { } } let serviceContainer: TypeMoq.IMock; - let formattingSettings: TypeMoq.IMock; let unitTestSettings: TypeMoq.IMock; let configService: TypeMoq.IMock; let productInstaller: ProductInstaller; @@ -54,12 +44,10 @@ suite('Product Path', () => { } serviceContainer = TypeMoq.Mock.ofType(); configService = TypeMoq.Mock.ofType(); - formattingSettings = TypeMoq.Mock.ofType(); unitTestSettings = TypeMoq.Mock.ofType(); productInstaller = new ProductInstaller(serviceContainer.object); const pythonSettings = TypeMoq.Mock.ofType(); - pythonSettings.setup((p) => p.formatting).returns(() => formattingSettings.object); pythonSettings.setup((p) => p.testing).returns(() => unitTestSettings.object); configService .setup((s) => s.getSettings(TypeMoq.It.isValue(resource))) @@ -99,37 +87,6 @@ suite('Product Path', () => { }); const productType = new ProductService().getProductType(product.value); switch (productType) { - case ProductType.Formatter: { - test(`Ensure path is returned for ${product.name} (${ - resource ? 'With a resource' : 'without a resource' - })`, async () => { - const productPathService = new FormatterProductPathService(serviceContainer.object); - const formatterHelper = TypeMoq.Mock.ofType(); - const expectedPath = 'Some Path'; - serviceContainer - .setup((s) => s.get(TypeMoq.It.isValue(IFormatterHelper), TypeMoq.It.isAny())) - .returns(() => formatterHelper.object); - formattingSettings - .setup((f) => f.autopep8Path) - .returns(() => expectedPath) - .verifiable(TypeMoq.Times.atLeastOnce()); - formatterHelper - .setup((f) => f.getSettingsPropertyNames(TypeMoq.It.isValue(product.value))) - .returns(() => { - return { - pathName: 'autopep8Path', - argsName: 'autopep8Args', - }; - }) - .verifiable(TypeMoq.Times.once()); - - const value = productPathService.getExecutableNameFromSettings(product.value, resource); - expect(value).to.be.equal(expectedPath); - formattingSettings.verifyAll(); - formatterHelper.verifyAll(); - }); - break; - } case ProductType.Linter: { test(`Ensure path is returned for ${product.name} (${ resource ? 'With a resource' : 'without a resource' diff --git a/src/test/common/installer/serviceRegistry.unit.test.ts b/src/test/common/installer/serviceRegistry.unit.test.ts index a23cff298d6c..5b971790fa9a 100644 --- a/src/test/common/installer/serviceRegistry.unit.test.ts +++ b/src/test/common/installer/serviceRegistry.unit.test.ts @@ -10,7 +10,6 @@ import { PipEnvInstaller } from '../../../client/common/installer/pipEnvInstalle import { PipInstaller } from '../../../client/common/installer/pipInstaller'; import { PoetryInstaller } from '../../../client/common/installer/poetryInstaller'; import { - FormatterProductPathService, LinterProductPathService, TestFrameworkProductPathService, } from '../../../client/common/installer/productPath'; @@ -46,13 +45,6 @@ suite('Common installer Service Registry', () => { ), ).once(); verify(serviceManager.addSingleton(IProductService, ProductService)).once(); - verify( - serviceManager.addSingleton( - IProductPathService, - FormatterProductPathService, - ProductType.Formatter, - ), - ).once(); verify( serviceManager.addSingleton( IProductPathService, diff --git a/src/test/common/moduleInstaller.test.ts b/src/test/common/moduleInstaller.test.ts index a6b647ad181d..2f73bc520307 100644 --- a/src/test/common/moduleInstaller.test.ts +++ b/src/test/common/moduleInstaller.test.ts @@ -147,7 +147,6 @@ suite('Module Installer', () => { ioc.registerUnitTestTypes(); ioc.registerVariableTypes(); ioc.registerLinterTypes(); - ioc.registerFormatterTypes(); ioc.registerInterpreterStorageTypes(); ioc.serviceManager.addSingleton(IPersistentStateFactory, PersistentStateFactory); diff --git a/src/test/format/extension.format.test.ts b/src/test/format/extension.format.test.ts deleted file mode 100644 index 40131be24ec2..000000000000 --- a/src/test/format/extension.format.test.ts +++ /dev/null @@ -1,205 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -'use strict'; - -import * as fs from 'fs-extra'; -import * as path from 'path'; -import { CancellationTokenSource, Position, Uri, window, workspace } from 'vscode'; -import { IProcessServiceFactory } from '../../client/common/process/types'; -import { AutoPep8Formatter } from '../../client/formatters/autoPep8Formatter'; -import { BlackFormatter } from '../../client/formatters/blackFormatter'; -import { YapfFormatter } from '../../client/formatters/yapfFormatter'; -import { isPythonVersionInProcess } from '../common'; -import { closeActiveWindows, initialize, initializeTest } from '../initialize'; -import { MockProcessService } from '../mocks/proc'; -import { registerForIOC } from '../pythonEnvironments/legacyIOC'; -import { UnitTestIocContainer } from '../testing/serviceRegistry'; -import { compareFiles } from '../textUtils'; - -const ch = window.createOutputChannel('Tests'); -const formatFilesPath = path.join(__dirname, '..', '..', '..', 'src', 'test', 'pythonFiles', 'formatting'); -const workspaceRootPath = path.join(__dirname, '..', '..', '..', 'src', 'test'); -const originalUnformattedFile = path.join(formatFilesPath, 'fileToFormat.py'); - -const autoPep8FileToFormat = path.join(formatFilesPath, 'autoPep8FileToFormat.py'); -const autoPep8Formatted = path.join(formatFilesPath, 'autoPep8Formatted.py'); -const blackFileToFormat = path.join(formatFilesPath, 'blackFileToFormat.py'); -const blackFormatted = path.join(formatFilesPath, 'blackFormatted.py'); -const yapfFileToFormat = path.join(formatFilesPath, 'yapfFileToFormat.py'); -const yapfFormatted = path.join(formatFilesPath, 'yapfFormatted.py'); - -let formattedYapf = ''; -let formattedBlack = ''; -let formattedAutoPep8 = ''; - -suite('Formatting - General', () => { - let ioc: UnitTestIocContainer; - - suiteSetup(async function () { - // https://github.com/microsoft/vscode-python/issues/12564 - // Skipping one test in the file is resulting in the next one failing, so skipping the entire suiteuntil further investigation. - - return this.skip(); - await initialize(); - await initializeDI(); - [autoPep8FileToFormat, blackFileToFormat, yapfFileToFormat].forEach((file) => { - fs.copySync(originalUnformattedFile, file, { overwrite: true }); - }); - formattedYapf = fs.readFileSync(yapfFormatted).toString(); - formattedAutoPep8 = fs.readFileSync(autoPep8Formatted).toString(); - formattedBlack = fs.readFileSync(blackFormatted).toString(); - }); - - async function formattingTestIsBlackSupported(): Promise { - const processService = await ioc.serviceContainer - .get(IProcessServiceFactory) - .create(Uri.file(workspaceRootPath)); - return !(await isPythonVersionInProcess(processService, '2', '3.0', '3.1', '3.2', '3.3', '3.4', '3.5')); - } - - setup(async () => { - await initializeTest(); - await initializeDI(); - }); - suiteTeardown(async () => { - [autoPep8FileToFormat, blackFileToFormat, yapfFileToFormat].forEach((file) => { - if (fs.existsSync(file)) { - fs.unlinkSync(file); - } - }); - ch.dispose(); - await closeActiveWindows(); - }); - teardown(async () => { - await ioc.dispose(); - }); - - async function initializeDI() { - ioc = new UnitTestIocContainer(); - ioc.registerCommonTypes(); - ioc.registerVariableTypes(); - ioc.registerUnitTestTypes(); - ioc.registerFormatterTypes(); - ioc.registerInterpreterStorageTypes(); - - // Mocks. - ioc.registerMockProcessTypes(); - await ioc.registerMockInterpreterTypes(); - - await registerForIOC(ioc.serviceManager, ioc.serviceContainer); - } - - async function injectFormatOutput(outputFileName: string) { - const procService = (await ioc.serviceContainer - .get(IProcessServiceFactory) - .create()) as MockProcessService; - procService.onExecObservable((_file, args, _options, callback) => { - if (args.indexOf('--diff') >= 0) { - callback({ - out: fs.readFileSync(path.join(formatFilesPath, outputFileName), 'utf8'), - source: 'stdout', - }); - } - }); - } - - async function testFormatting( - formatter: AutoPep8Formatter | BlackFormatter | YapfFormatter, - formattedContents: string, - fileToFormat: string, - outputFileName: string, - ) { - const textDocument = await workspace.openTextDocument(fileToFormat); - const textEditor = await window.showTextDocument(textDocument); - const options = { - insertSpaces: textEditor.options.insertSpaces! as boolean, - tabSize: textEditor.options.tabSize! as number, - }; - - await injectFormatOutput(outputFileName); - - const edits = await formatter.formatDocument(textDocument, options, new CancellationTokenSource().token); - await textEditor.edit((editBuilder) => { - edits.forEach((edit) => editBuilder.replace(edit.range, edit.newText)); - }); - compareFiles(formattedContents, textEditor.document.getText()); - } - - test('AutoPep8', async function () { - // https://github.com/microsoft/vscode-python/issues/12564 - - return this.skip(); - await testFormatting( - new AutoPep8Formatter(ioc.serviceContainer), - formattedAutoPep8, - autoPep8FileToFormat, - 'autopep8.output', - ); - }); - - test('Black', async function () { - // https://github.com/microsoft/vscode-python/issues/12564 - - return this.skip(); - if (!(await formattingTestIsBlackSupported())) { - // Skip for versions of python below 3.6, as Black doesn't support them at all. - - return this.skip(); - } - await testFormatting( - new BlackFormatter(ioc.serviceContainer), - formattedBlack, - blackFileToFormat, - 'black.output', - ); - }); - test('Yapf', async () => - testFormatting(new YapfFormatter(ioc.serviceContainer), formattedYapf, yapfFileToFormat, 'yapf.output')); - - test('Yapf on dirty file', async () => { - const sourceDir = path.join(__dirname, '..', '..', '..', 'src', 'test', 'pythonFiles', 'formatting'); - const targetDir = path.join(__dirname, '..', 'pythonFiles', 'formatting'); - - const originalName = 'formatWhenDirty.py'; - const resultsName = 'formatWhenDirtyResult.py'; - const fileToFormat = path.join(targetDir, originalName); - const formattedFile = path.join(targetDir, resultsName); - - if (!fs.pathExistsSync(targetDir)) { - fs.mkdirpSync(targetDir); - } - fs.copySync(path.join(sourceDir, originalName), fileToFormat, { overwrite: true }); - fs.copySync(path.join(sourceDir, resultsName), formattedFile, { overwrite: true }); - - const textDocument = await workspace.openTextDocument(fileToFormat); - const textEditor = await window.showTextDocument(textDocument); - await textEditor.edit((builder) => { - // Make file dirty. Trailing blanks will be removed. - builder.insert(new Position(0, 0), '\n \n'); - }); - - const dir = path.dirname(fileToFormat); - const configFile = path.join(dir, '.style.yapf'); - try { - // Create yapf configuration file - const content = '[style]\nbased_on_style = pep8\nindent_width=5\n'; - fs.writeFileSync(configFile, content); - - const options = { insertSpaces: textEditor.options.insertSpaces! as boolean, tabSize: 1 }; - const formatter = new YapfFormatter(ioc.serviceContainer); - const edits = await formatter.formatDocument(textDocument, options, new CancellationTokenSource().token); - await textEditor.edit((editBuilder) => { - edits.forEach((edit) => editBuilder.replace(edit.range, edit.newText)); - }); - - const expected = fs.readFileSync(formattedFile).toString(); - const actual = textEditor.document.getText(); - compareFiles(expected, actual); - } finally { - if (fs.existsSync(configFile)) { - fs.unlinkSync(configFile); - } - } - }); -}); diff --git a/src/test/format/format.helper.test.ts b/src/test/format/format.helper.test.ts deleted file mode 100644 index 50000f1af867..000000000000 --- a/src/test/format/format.helper.test.ts +++ /dev/null @@ -1,117 +0,0 @@ -import * as assert from 'assert'; -import * as TypeMoq from 'typemoq'; -import { IConfigurationService, IFormattingSettings, Product } from '../../client/common/types'; -import * as EnumEx from '../../client/common/utils/enum'; -import { FormatterHelper } from '../../client/formatters/helper'; -import { FormatterId } from '../../client/formatters/types'; -import { getExtensionSettings } from '../extensionSettings'; -import { initialize } from '../initialize'; -import { UnitTestIocContainer } from '../testing/serviceRegistry'; - -suite('Formatting - Helper', () => { - let ioc: UnitTestIocContainer; - let formatHelper: FormatterHelper; - - suiteSetup(initialize); - setup(() => { - ioc = new UnitTestIocContainer(); - - const config = TypeMoq.Mock.ofType(); - config.setup((x) => x.getSettings(TypeMoq.It.isAny())).returns(() => getExtensionSettings(undefined)); - - ioc.serviceManager.addSingletonInstance(IConfigurationService, config.object); - formatHelper = new FormatterHelper(ioc.serviceManager); - }); - - test('Ensure product is set in Execution Info', async () => { - [Product.autopep8, Product.black, Product.yapf].forEach((formatter) => { - const info = formatHelper.getExecutionInfo(formatter, []); - assert.strictEqual( - info.product, - formatter, - `Incorrect products for ${formatHelper.translateToId(formatter)}`, - ); - }); - }); - - test('Ensure executable is set in Execution Info', async () => { - const settings = getExtensionSettings(undefined); - - [Product.autopep8, Product.black, Product.yapf].forEach((formatter) => { - const info = formatHelper.getExecutionInfo(formatter, []); - const names = formatHelper.getSettingsPropertyNames(formatter); - const execPath = settings.formatting[names.pathName] as string; - - assert.strictEqual( - info.execPath, - execPath, - `Incorrect executable paths for product ${formatHelper.translateToId(formatter)}`, - ); - }); - }); - - test('Ensure arguments are set in Execution Info', async () => { - const settings = getExtensionSettings(undefined); - const customArgs = ['1', '2', '3']; - - [Product.autopep8, Product.black, Product.yapf].forEach((formatter) => { - const names = formatHelper.getSettingsPropertyNames(formatter); - const args: string[] = Array.isArray(settings.formatting[names.argsName]) - ? (settings.formatting[names.argsName] as string[]) - : []; - const expectedArgs = args.concat(customArgs).join(','); - - assert.strictEqual( - expectedArgs.endsWith(customArgs.join(',')), - true, - `Incorrect custom arguments for product ${formatHelper.translateToId(formatter)}`, - ); - }); - }); - - test('Ensure correct setting names are returned', async () => { - [Product.autopep8, Product.black, Product.yapf].forEach((formatter) => { - const translatedId = formatHelper.translateToId(formatter)!; - const settings = { - argsName: `${translatedId}Args` as keyof IFormattingSettings, - pathName: `${translatedId}Path` as keyof IFormattingSettings, - }; - - assert.deepEqual( - formatHelper.getSettingsPropertyNames(formatter), - settings, - `Incorrect settings for product ${formatHelper.translateToId(formatter)}`, - ); - }); - }); - - test('Ensure translation of ids works', async () => { - const formatterMapping = new Map(); - formatterMapping.set(Product.autopep8, 'autopep8'); - formatterMapping.set(Product.black, 'black'); - formatterMapping.set(Product.yapf, 'yapf'); - - [Product.autopep8, Product.black, Product.yapf].forEach((formatter) => { - const translatedId = formatHelper.translateToId(formatter); - assert.strictEqual( - translatedId, - formatterMapping.get(formatter)!, - `Incorrect translation for product ${formatHelper.translateToId(formatter)}`, - ); - }); - }); - - EnumEx.getValues(Product).forEach((product) => { - const formatterMapping = new Map(); - formatterMapping.set(Product.autopep8, 'autopep8'); - formatterMapping.set(Product.black, 'black'); - formatterMapping.set(Product.yapf, 'yapf'); - if (formatterMapping.has(product)) { - return; - } - - test(`Ensure translation of ids throws exceptions for unknown formatters (${product})`, async () => { - assert.throws(() => formatHelper.translateToId(product)); - }); - }); -}); diff --git a/src/test/format/formatter.unit.test.ts b/src/test/format/formatter.unit.test.ts deleted file mode 100644 index 05970d0c71f6..000000000000 --- a/src/test/format/formatter.unit.test.ts +++ /dev/null @@ -1,171 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -'use strict'; - -import * as assert from 'assert'; -import * as path from 'path'; -import { anything, capture, instance, mock, when } from 'ts-mockito'; -import * as typemoq from 'typemoq'; -import { CancellationTokenSource, FormattingOptions, TextDocument, Uri } from 'vscode'; -import { ApplicationShell } from '../../client/common/application/applicationShell'; -import { IApplicationShell, IWorkspaceService } from '../../client/common/application/types'; -import { WorkspaceService } from '../../client/common/application/workspace'; -import { PythonSettings } from '../../client/common/configSettings'; -import { ConfigurationService } from '../../client/common/configuration/service'; -import { PythonToolExecutionService } from '../../client/common/process/pythonToolService'; -import { IPythonToolExecutionService } from '../../client/common/process/types'; -import { - ExecutionInfo, - IConfigurationService, - IDisposableRegistry, - IFormattingSettings, - ILogOutputChannel, - IPythonSettings, -} from '../../client/common/types'; -import { AutoPep8Formatter } from '../../client/formatters/autoPep8Formatter'; -import { BaseFormatter } from '../../client/formatters/baseFormatter'; -import { BlackFormatter } from '../../client/formatters/blackFormatter'; -import { FormatterHelper } from '../../client/formatters/helper'; -import { IFormatterHelper } from '../../client/formatters/types'; -import { YapfFormatter } from '../../client/formatters/yapfFormatter'; -import { ServiceContainer } from '../../client/ioc/container'; -import { IServiceContainer } from '../../client/ioc/types'; -import { noop } from '../core'; -import { MockOutputChannel } from '../mockClasses'; - -suite('Formatting - Test Arguments', () => { - let container: IServiceContainer; - let outputChannel: ILogOutputChannel; - let workspace: IWorkspaceService; - let settings: IPythonSettings; - const workspaceUri = Uri.file(__dirname); - let document: typemoq.IMock; - const docUri = Uri.file(__filename); - let pythonToolExecutionService: IPythonToolExecutionService; - const options: FormattingOptions = { insertSpaces: false, tabSize: 1 }; - const formattingSettingsWithPath: IFormattingSettings = { - autopep8Args: ['1', '2'], - autopep8Path: path.join('a', 'exe'), - blackArgs: ['1', '2'], - blackPath: path.join('a', 'exe'), - provider: '', - yapfArgs: ['1', '2'], - yapfPath: path.join('a', 'exe'), - }; - - const formattingSettingsWithModuleName: IFormattingSettings = { - autopep8Args: ['1', '2'], - autopep8Path: 'module_name', - blackArgs: ['1', '2'], - blackPath: 'module_name', - provider: '', - yapfArgs: ['1', '2'], - yapfPath: 'module_name', - }; - - setup(() => { - container = mock(ServiceContainer); - outputChannel = mock(MockOutputChannel); - workspace = mock(WorkspaceService); - settings = mock(PythonSettings); - document = typemoq.Mock.ofType(); - document.setup((doc) => doc.getText(typemoq.It.isAny())).returns(() => ''); - document.setup((doc) => doc.isDirty).returns(() => false); - document.setup((doc) => doc.fileName).returns(() => docUri.fsPath); - document.setup((doc) => doc.uri).returns(() => docUri); - pythonToolExecutionService = mock(PythonToolExecutionService); - - const configService = mock(ConfigurationService); - const formatterHelper = new FormatterHelper(instance(container)); - - const appShell = mock(ApplicationShell); - when(appShell.setStatusBarMessage(anything(), anything())).thenReturn({ dispose: noop }); - - when(configService.getSettings(anything())).thenReturn(instance(settings)); - when(workspace.getWorkspaceFolder(anything())).thenReturn({ name: '', index: 0, uri: workspaceUri }); - when(container.get(ILogOutputChannel)).thenReturn(instance(outputChannel)); - when(container.get(IApplicationShell)).thenReturn(instance(appShell)); - when(container.get(IFormatterHelper)).thenReturn(formatterHelper); - when(container.get(IWorkspaceService)).thenReturn(instance(workspace)); - when(container.get(IConfigurationService)).thenReturn(instance(configService)); - when(container.get(IPythonToolExecutionService)).thenReturn( - instance(pythonToolExecutionService), - ); - when(container.get(IDisposableRegistry)).thenReturn([]); - }); - - async function setupFormatter( - formatter: BaseFormatter, - formattingSettings: IFormattingSettings, - ): Promise { - const { token } = new CancellationTokenSource(); - when(settings.formatting).thenReturn(formattingSettings); - when(pythonToolExecutionService.exec(anything(), anything(), anything())).thenResolve({ stdout: '' }); - - await formatter.formatDocument(document.object, options, token); - - const args = capture(pythonToolExecutionService.exec).first(); - return args[0]; - } - test('Ensure blackPath and args used to launch the formatter', async () => { - const formatter = new BlackFormatter(instance(container)); - - const execInfo = await setupFormatter(formatter, formattingSettingsWithPath); - - assert.strictEqual(execInfo.execPath, formattingSettingsWithPath.blackPath); - assert.strictEqual(execInfo.moduleName, undefined); - assert.deepEqual( - execInfo.args, - formattingSettingsWithPath.blackArgs.concat(['--diff', '--quiet', docUri.fsPath]), - ); - }); - test('Ensure black modulename and args used to launch the formatter', async () => { - const formatter = new BlackFormatter(instance(container)); - - const execInfo = await setupFormatter(formatter, formattingSettingsWithModuleName); - - assert.strictEqual(execInfo.execPath, formattingSettingsWithModuleName.blackPath); - assert.strictEqual(execInfo.moduleName, formattingSettingsWithModuleName.blackPath); - assert.deepEqual( - execInfo.args, - formattingSettingsWithPath.blackArgs.concat(['--diff', '--quiet', docUri.fsPath]), - ); - }); - test('Ensure autopep8path and args used to launch the formatter', async () => { - const formatter = new AutoPep8Formatter(instance(container)); - - const execInfo = await setupFormatter(formatter, formattingSettingsWithPath); - - assert.strictEqual(execInfo.execPath, formattingSettingsWithPath.autopep8Path); - assert.strictEqual(execInfo.moduleName, undefined); - assert.deepEqual(execInfo.args, formattingSettingsWithPath.autopep8Args.concat(['--diff', docUri.fsPath])); - }); - test('Ensure autpep8 modulename and args used to launch the formatter', async () => { - const formatter = new AutoPep8Formatter(instance(container)); - - const execInfo = await setupFormatter(formatter, formattingSettingsWithModuleName); - - assert.strictEqual(execInfo.execPath, formattingSettingsWithModuleName.autopep8Path); - assert.strictEqual(execInfo.moduleName, formattingSettingsWithModuleName.autopep8Path); - assert.deepEqual(execInfo.args, formattingSettingsWithPath.autopep8Args.concat(['--diff', docUri.fsPath])); - }); - test('Ensure yapfpath and args used to launch the formatter', async () => { - const formatter = new YapfFormatter(instance(container)); - - const execInfo = await setupFormatter(formatter, formattingSettingsWithPath); - - assert.strictEqual(execInfo.execPath, formattingSettingsWithPath.yapfPath); - assert.strictEqual(execInfo.moduleName, undefined); - assert.deepEqual(execInfo.args, formattingSettingsWithPath.yapfArgs.concat(['--diff', docUri.fsPath])); - }); - test('Ensure yapf modulename and args used to launch the formatter', async () => { - const formatter = new YapfFormatter(instance(container)); - - const execInfo = await setupFormatter(formatter, formattingSettingsWithModuleName); - - assert.strictEqual(execInfo.execPath, formattingSettingsWithModuleName.yapfPath); - assert.strictEqual(execInfo.moduleName, formattingSettingsWithModuleName.yapfPath); - assert.deepEqual(execInfo.args, formattingSettingsWithPath.yapfArgs.concat(['--diff', docUri.fsPath])); - }); -}); diff --git a/src/test/linters/lint.multiroot.test.ts b/src/test/linters/lint.multiroot.test.ts index f89ee86c0b42..5c1cae31d158 100644 --- a/src/test/linters/lint.multiroot.test.ts +++ b/src/test/linters/lint.multiroot.test.ts @@ -3,11 +3,7 @@ import * as path from 'path'; import { CancellationTokenSource, ConfigurationTarget, Uri, workspace } from 'vscode'; import { LanguageServerType } from '../../client/activation/types'; import { PythonSettings } from '../../client/common/configSettings'; -import { - FormatterProductPathService, - LinterProductPathService, - TestFrameworkProductPathService, -} from '../../client/common/installer/productPath'; +import { LinterProductPathService, TestFrameworkProductPathService } from '../../client/common/installer/productPath'; import { ProductService } from '../../client/common/installer/productService'; import { IProductPathService, IProductService } from '../../client/common/installer/types'; import { IConfigurationService, Product, ProductType } from '../../client/common/types'; @@ -72,11 +68,6 @@ suite('Multiroot Linting', () => { ); ioc.registerInterpreterStorageTypes(); ioc.serviceManager.addSingletonInstance(IProductService, new ProductService()); - ioc.serviceManager.addSingleton( - IProductPathService, - FormatterProductPathService, - ProductType.Formatter, - ); ioc.serviceManager.addSingleton( IProductPathService, LinterProductPathService, diff --git a/src/test/linters/lint.test.ts b/src/test/linters/lint.test.ts index d2eef3c9e321..837830f0c499 100644 --- a/src/test/linters/lint.test.ts +++ b/src/test/linters/lint.test.ts @@ -6,11 +6,7 @@ import * as assert from 'assert'; import { ConfigurationTarget } from 'vscode'; import { Product } from '../../client/common/installer/productInstaller'; -import { - FormatterProductPathService, - LinterProductPathService, - TestFrameworkProductPathService, -} from '../../client/common/installer/productPath'; +import { LinterProductPathService, TestFrameworkProductPathService } from '../../client/common/installer/productPath'; import { ProductService } from '../../client/common/installer/productService'; import { IProductPathService, IProductService } from '../../client/common/installer/types'; import { IConfigurationService, ILintingSettings, ProductType } from '../../client/common/types'; @@ -50,11 +46,6 @@ suite('Linting Settings', () => { configService = ioc.serviceContainer.get(IConfigurationService); linterManager = new LinterManager(configService); ioc.serviceManager.addSingletonInstance(IProductService, new ProductService()); - ioc.serviceManager.addSingleton( - IProductPathService, - FormatterProductPathService, - ProductType.Formatter, - ); ioc.serviceManager.addSingleton( IProductPathService, LinterProductPathService, diff --git a/src/test/serviceRegistry.ts b/src/test/serviceRegistry.ts index c20a84b1e25a..e7b11d2b745b 100644 --- a/src/test/serviceRegistry.ts +++ b/src/test/serviceRegistry.ts @@ -33,7 +33,6 @@ import { ITestOutputChannel, } from '../client/common/types'; import { registerTypes as variableRegisterTypes } from '../client/common/variables/serviceRegistry'; -import { registerTypes as formattersRegisterTypes } from '../client/formatters/serviceRegistry'; import { EnvironmentActivationService } from '../client/interpreter/activation/service'; import { IEnvironmentActivationService } from '../client/interpreter/activation/types'; import { @@ -147,10 +146,6 @@ export class IocContainer { lintersRegisterTypes(this.serviceManager); } - public registerFormatterTypes(): void { - formattersRegisterTypes(this.serviceManager); - } - public registerPlatformTypes(): void { platformRegisterTypes(this.serviceManager); } From eade53b94118137f703ca3f036a6ab5be4a5ef1b Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Wed, 11 Oct 2023 11:28:44 -0700 Subject: [PATCH 2/3] Add logging and notification when python extension set as default formatter --- src/client/extensionActivation.ts | 4 +- src/client/logging/settingLogs.ts | 42 +++ .../prompts/installFormatterPrompt.ts | 145 -------- src/client/providers/prompts/promptUtils.ts | 38 -- src/client/providers/prompts/types.ts | 12 - src/client/providers/serviceRegistry.ts | 3 - .../installFormatterPrompt.unit.test.ts | 335 ------------------ 7 files changed, 44 insertions(+), 535 deletions(-) create mode 100644 src/client/logging/settingLogs.ts delete mode 100644 src/client/providers/prompts/installFormatterPrompt.ts delete mode 100644 src/client/providers/prompts/promptUtils.ts delete mode 100644 src/client/providers/prompts/types.ts delete mode 100644 src/test/providers/prompt/installFormatterPrompt.unit.test.ts diff --git a/src/client/extensionActivation.ts b/src/client/extensionActivation.ts index 7730427878f7..0d3b04d9bb8c 100644 --- a/src/client/extensionActivation.ts +++ b/src/client/extensionActivation.ts @@ -49,10 +49,10 @@ import { IDebugSessionEventHandlers } from './debugger/extension/hooks/types'; import { WorkspaceService } from './common/application/workspace'; import { DynamicPythonDebugConfigurationService } from './debugger/extension/configuration/dynamicdebugConfigurationService'; import { IInterpreterQuickPick } from './interpreter/configuration/types'; -import { registerInstallFormatterPrompt } from './providers/prompts/installFormatterPrompt'; import { registerAllCreateEnvironmentFeatures } from './pythonEnvironments/creation/registrations'; import { registerCreateEnvironmentTriggers } from './pythonEnvironments/creation/createEnvironmentTrigger'; import { initializePersistentStateForTriggers } from './common/persistentState'; +import { logAndNotifyOnFormatterSetting } from './logging/settingLogs'; export async function activateComponents( // `ext` is passed to any extra activation funcs. @@ -185,7 +185,7 @@ async function activateLegacy(ext: ExtensionState): Promise { ), ); - registerInstallFormatterPrompt(serviceContainer); + logAndNotifyOnFormatterSetting(); registerCreateEnvironmentTriggers(disposables); initializePersistentStateForTriggers(ext.context); } diff --git a/src/client/logging/settingLogs.ts b/src/client/logging/settingLogs.ts new file mode 100644 index 000000000000..f892dea99b76 --- /dev/null +++ b/src/client/logging/settingLogs.ts @@ -0,0 +1,42 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { l10n } from 'vscode'; +import { traceError, traceInfo } from '.'; +import { Commands, PVSC_EXTENSION_ID } from '../common/constants'; +import { showErrorMessage } from '../common/vscodeApis/windowApis'; +import { getConfiguration, getWorkspaceFolders } from '../common/vscodeApis/workspaceApis'; +import { Common } from '../common/utils/localize'; +import { executeCommand } from '../common/vscodeApis/commandApis'; + +export function logAndNotifyOnFormatterSetting(): void { + getWorkspaceFolders()?.forEach(async (workspace) => { + let config = getConfiguration('editor', { uri: workspace.uri, languageId: 'python' }); + if (!config) { + config = getConfiguration('editor', workspace.uri); + if (!config) { + traceError('Unable to get editor configuration'); + } + } + const formatter = config.get('defaultFormatter', ''); + traceInfo(`Default formatter is set to ${formatter} for workspace ${workspace.uri.fsPath}`); + if (formatter === PVSC_EXTENSION_ID) { + traceError('Formatting feature is moved to separate extensions depending on the formatter you prefer.'); + traceError('Please install the formatter extension you prefer and set it as the default formatter.'); + traceError('For `autopep8` use: https://marketplace.visualstudio.com/items?itemName=ms-python.autopep8'); + traceError( + 'For `black` use: https://marketplace.visualstudio.com/items?itemName=ms-python.black-formatter', + ); + traceError('For `yapf` use: https://marketplace.visualstudio.com/items?itemName=eeyore.yapf'); + const response = await showErrorMessage( + l10n.t( + 'Formatting feature is moved to separate extensions depending on the formatter you prefer. Please install the formatter extension you prefer and set it as the default formatter.', + ), + Common.showLogs, + ); + if (response === Common.showLogs) { + executeCommand(Commands.ViewOutput); + } + } + }); +} diff --git a/src/client/providers/prompts/installFormatterPrompt.ts b/src/client/providers/prompts/installFormatterPrompt.ts deleted file mode 100644 index 5743f8402053..000000000000 --- a/src/client/providers/prompts/installFormatterPrompt.ts +++ /dev/null @@ -1,145 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -import { Uri } from 'vscode'; -import { inject, injectable } from 'inversify'; -import { IDisposableRegistry } from '../../common/types'; -import { Common, ToolsExtensions } from '../../common/utils/localize'; -import { isExtensionEnabled } from '../../common/vscodeApis/extensionsApi'; -import { showInformationMessage } from '../../common/vscodeApis/windowApis'; -import { getConfiguration, onDidSaveTextDocument } from '../../common/vscodeApis/workspaceApis'; -import { IServiceContainer } from '../../ioc/types'; -import { - doNotShowPromptState, - inFormatterExtensionExperiment, - installFormatterExtension, - updateDefaultFormatter, -} from './promptUtils'; -import { AUTOPEP8_EXTENSION, BLACK_EXTENSION, IInstallFormatterPrompt } from './types'; - -const SHOW_FORMATTER_INSTALL_PROMPT_DONOTSHOW_KEY = 'showFormatterExtensionInstallPrompt'; - -@injectable() -export class InstallFormatterPrompt implements IInstallFormatterPrompt { - private currentlyShown = false; - - constructor(@inject(IServiceContainer) private readonly serviceContainer: IServiceContainer) {} - - /* - * This method is called when the user saves a python file or a cell. - * Returns true if an extension was selected. Otherwise returns false. - */ - public async showInstallFormatterPrompt(resource?: Uri): Promise { - if (!inFormatterExtensionExperiment(this.serviceContainer)) { - return false; - } - - const promptState = doNotShowPromptState(SHOW_FORMATTER_INSTALL_PROMPT_DONOTSHOW_KEY, this.serviceContainer); - if (this.currentlyShown || promptState.value) { - return false; - } - - const config = getConfiguration('python', resource); - const formatter = config.get('formatting.provider', 'none'); - if (!['autopep8', 'black'].includes(formatter)) { - return false; - } - - const editorConfig = getConfiguration('editor', { uri: resource, languageId: 'python' }); - const defaultFormatter = editorConfig.get('defaultFormatter', ''); - if ([BLACK_EXTENSION, AUTOPEP8_EXTENSION].includes(defaultFormatter)) { - return false; - } - - const black = isExtensionEnabled(BLACK_EXTENSION); - const autopep8 = isExtensionEnabled(AUTOPEP8_EXTENSION); - - let selection: string | undefined; - - if (black || autopep8) { - this.currentlyShown = true; - if (black && autopep8) { - selection = await showInformationMessage( - ToolsExtensions.selectMultipleFormattersPrompt, - 'Black', - 'Autopep8', - Common.doNotShowAgain, - ); - } else if (black) { - selection = await showInformationMessage( - ToolsExtensions.selectBlackFormatterPrompt, - Common.bannerLabelYes, - Common.doNotShowAgain, - ); - if (selection === Common.bannerLabelYes) { - selection = 'Black'; - } - } else if (autopep8) { - selection = await showInformationMessage( - ToolsExtensions.selectAutopep8FormatterPrompt, - Common.bannerLabelYes, - Common.doNotShowAgain, - ); - if (selection === Common.bannerLabelYes) { - selection = 'Autopep8'; - } - } - } else if (formatter === 'black' && !black) { - this.currentlyShown = true; - selection = await showInformationMessage( - ToolsExtensions.installBlackFormatterPrompt, - 'Black', - 'Autopep8', - Common.doNotShowAgain, - ); - } else if (formatter === 'autopep8' && !autopep8) { - this.currentlyShown = true; - selection = await showInformationMessage( - ToolsExtensions.installAutopep8FormatterPrompt, - 'Black', - 'Autopep8', - Common.doNotShowAgain, - ); - } - - let userSelectedAnExtension = false; - if (selection === 'Black') { - if (black) { - userSelectedAnExtension = true; - await updateDefaultFormatter(BLACK_EXTENSION, resource); - } else { - userSelectedAnExtension = true; - await installFormatterExtension(BLACK_EXTENSION, resource); - } - } else if (selection === 'Autopep8') { - if (autopep8) { - userSelectedAnExtension = true; - await updateDefaultFormatter(AUTOPEP8_EXTENSION, resource); - } else { - userSelectedAnExtension = true; - await installFormatterExtension(AUTOPEP8_EXTENSION, resource); - } - } else if (selection === Common.doNotShowAgain) { - userSelectedAnExtension = false; - await promptState.updateValue(true); - } else { - userSelectedAnExtension = false; - } - - this.currentlyShown = false; - return userSelectedAnExtension; - } -} - -export function registerInstallFormatterPrompt(serviceContainer: IServiceContainer): void { - const disposables = serviceContainer.get(IDisposableRegistry); - const installFormatterPrompt = serviceContainer.get(IInstallFormatterPrompt); - disposables.push( - onDidSaveTextDocument(async (e) => { - const editorConfig = getConfiguration('editor', { uri: e.uri, languageId: 'python' }); - if (e.languageId === 'python' && editorConfig.get('formatOnSave')) { - await installFormatterPrompt.showInstallFormatterPrompt(e.uri); - } - }), - ); -} diff --git a/src/client/providers/prompts/promptUtils.ts b/src/client/providers/prompts/promptUtils.ts deleted file mode 100644 index 05b1b28f061a..000000000000 --- a/src/client/providers/prompts/promptUtils.ts +++ /dev/null @@ -1,38 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -import { ConfigurationTarget, Uri } from 'vscode'; -import { ShowFormatterExtensionPrompt } from '../../common/experiments/groups'; -import { IExperimentService, IPersistentState, IPersistentStateFactory } from '../../common/types'; -import { executeCommand } from '../../common/vscodeApis/commandApis'; -import { isInsider } from '../../common/vscodeApis/extensionsApi'; -import { getConfiguration, getWorkspaceFolder } from '../../common/vscodeApis/workspaceApis'; -import { IServiceContainer } from '../../ioc/types'; - -export function inFormatterExtensionExperiment(serviceContainer: IServiceContainer): boolean { - const experiment = serviceContainer.get(IExperimentService); - return experiment.inExperimentSync(ShowFormatterExtensionPrompt.experiment); -} - -export function doNotShowPromptState(key: string, serviceContainer: IServiceContainer): IPersistentState { - const persistFactory = serviceContainer.get(IPersistentStateFactory); - const promptState = persistFactory.createWorkspacePersistentState(key, false); - return promptState; -} - -export async function updateDefaultFormatter(extensionId: string, resource?: Uri): Promise { - const scope = getWorkspaceFolder(resource) ? ConfigurationTarget.Workspace : ConfigurationTarget.Global; - - const config = getConfiguration('python', resource); - const editorConfig = getConfiguration('editor', { uri: resource, languageId: 'python' }); - await editorConfig.update('defaultFormatter', extensionId, scope, true); - await config.update('formatting.provider', 'none', scope); -} - -export async function installFormatterExtension(extensionId: string, resource?: Uri): Promise { - await executeCommand('workbench.extensions.installExtension', extensionId, { - installPreReleaseVersion: isInsider(), - }); - - await updateDefaultFormatter(extensionId, resource); -} diff --git a/src/client/providers/prompts/types.ts b/src/client/providers/prompts/types.ts deleted file mode 100644 index 4edaadb46b46..000000000000 --- a/src/client/providers/prompts/types.ts +++ /dev/null @@ -1,12 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -import { Uri } from 'vscode'; - -export const BLACK_EXTENSION = 'ms-python.black-formatter'; -export const AUTOPEP8_EXTENSION = 'ms-python.autopep8'; - -export const IInstallFormatterPrompt = Symbol('IInstallFormatterPrompt'); -export interface IInstallFormatterPrompt { - showInstallFormatterPrompt(resource?: Uri): Promise; -} diff --git a/src/client/providers/serviceRegistry.ts b/src/client/providers/serviceRegistry.ts index 70fc6dc34135..a96ec14ff5e9 100644 --- a/src/client/providers/serviceRegistry.ts +++ b/src/client/providers/serviceRegistry.ts @@ -6,13 +6,10 @@ import { IExtensionSingleActivationService } from '../activation/types'; import { IServiceManager } from '../ioc/types'; import { CodeActionProviderService } from './codeActionProvider/main'; -import { InstallFormatterPrompt } from './prompts/installFormatterPrompt'; -import { IInstallFormatterPrompt } from './prompts/types'; export function registerTypes(serviceManager: IServiceManager): void { serviceManager.addSingleton( IExtensionSingleActivationService, CodeActionProviderService, ); - serviceManager.addSingleton(IInstallFormatterPrompt, InstallFormatterPrompt); } diff --git a/src/test/providers/prompt/installFormatterPrompt.unit.test.ts b/src/test/providers/prompt/installFormatterPrompt.unit.test.ts deleted file mode 100644 index fbd3a72d8cef..000000000000 --- a/src/test/providers/prompt/installFormatterPrompt.unit.test.ts +++ /dev/null @@ -1,335 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -import { assert } from 'chai'; -import * as sinon from 'sinon'; -import * as TypeMoq from 'typemoq'; -import { WorkspaceConfiguration } from 'vscode'; -import { IPersistentState } from '../../../client/common/types'; -import * as workspaceApis from '../../../client/common/vscodeApis/workspaceApis'; -import * as windowApis from '../../../client/common/vscodeApis/windowApis'; -import * as extensionsApi from '../../../client/common/vscodeApis/extensionsApi'; -import { IServiceContainer } from '../../../client/ioc/types'; -import { InstallFormatterPrompt } from '../../../client/providers/prompts/installFormatterPrompt'; -import * as promptUtils from '../../../client/providers/prompts/promptUtils'; -import { AUTOPEP8_EXTENSION, BLACK_EXTENSION, IInstallFormatterPrompt } from '../../../client/providers/prompts/types'; -import { Common, ToolsExtensions } from '../../../client/common/utils/localize'; - -suite('Formatter Extension prompt tests', () => { - let inFormatterExtensionExperimentStub: sinon.SinonStub; - let doNotShowPromptStateStub: sinon.SinonStub; - let prompt: IInstallFormatterPrompt; - let serviceContainer: TypeMoq.IMock; - let persistState: TypeMoq.IMock>; - let getConfigurationStub: sinon.SinonStub; - let isExtensionEnabledStub: sinon.SinonStub; - let pythonConfig: TypeMoq.IMock; - let editorConfig: TypeMoq.IMock; - let showInformationMessageStub: sinon.SinonStub; - let installFormatterExtensionStub: sinon.SinonStub; - let updateDefaultFormatterStub: sinon.SinonStub; - - setup(() => { - inFormatterExtensionExperimentStub = sinon.stub(promptUtils, 'inFormatterExtensionExperiment'); - inFormatterExtensionExperimentStub.returns(true); - - doNotShowPromptStateStub = sinon.stub(promptUtils, 'doNotShowPromptState'); - persistState = TypeMoq.Mock.ofType>(); - doNotShowPromptStateStub.returns(persistState.object); - - getConfigurationStub = sinon.stub(workspaceApis, 'getConfiguration'); - pythonConfig = TypeMoq.Mock.ofType(); - editorConfig = TypeMoq.Mock.ofType(); - getConfigurationStub.callsFake((section: string) => { - if (section === 'python') { - return pythonConfig.object; - } - return editorConfig.object; - }); - isExtensionEnabledStub = sinon.stub(extensionsApi, 'isExtensionEnabled'); - showInformationMessageStub = sinon.stub(windowApis, 'showInformationMessage'); - installFormatterExtensionStub = sinon.stub(promptUtils, 'installFormatterExtension'); - updateDefaultFormatterStub = sinon.stub(promptUtils, 'updateDefaultFormatter'); - - serviceContainer = TypeMoq.Mock.ofType(); - - prompt = new InstallFormatterPrompt(serviceContainer.object); - }); - - teardown(() => { - sinon.restore(); - }); - - test('Not in experiment', async () => { - inFormatterExtensionExperimentStub.returns(false); - - await prompt.showInstallFormatterPrompt(); - assert.isTrue(doNotShowPromptStateStub.notCalled); - }); - - test('Do not show was set', async () => { - persistState.setup((p) => p.value).returns(() => true); - - await prompt.showInstallFormatterPrompt(); - assert.isTrue(getConfigurationStub.notCalled); - }); - - test('Formatting provider is set to none', async () => { - persistState.setup((p) => p.value).returns(() => false); - pythonConfig.setup((p) => p.get('formatting.provider', TypeMoq.It.isAny())).returns(() => 'none'); - - await prompt.showInstallFormatterPrompt(); - assert.isTrue(isExtensionEnabledStub.notCalled); - }); - - test('Formatting provider is set to yapf', async () => { - persistState.setup((p) => p.value).returns(() => false); - pythonConfig.setup((p) => p.get('formatting.provider', TypeMoq.It.isAny())).returns(() => 'yapf'); - - await prompt.showInstallFormatterPrompt(); - assert.isTrue(isExtensionEnabledStub.notCalled); - }); - - test('Formatting provider is set to autopep8, and autopep8 extension is set as default formatter', async () => { - persistState.setup((p) => p.value).returns(() => false); - pythonConfig.setup((p) => p.get('formatting.provider', TypeMoq.It.isAny())).returns(() => 'autopep8'); - editorConfig.setup((p) => p.get('defaultFormatter', TypeMoq.It.isAny())).returns(() => AUTOPEP8_EXTENSION); - - await prompt.showInstallFormatterPrompt(); - assert.isTrue(isExtensionEnabledStub.notCalled); - }); - - test('Formatting provider is set to black, and black extension is set as default formatter', async () => { - persistState.setup((p) => p.value).returns(() => false); - pythonConfig.setup((p) => p.get('formatting.provider', TypeMoq.It.isAny())).returns(() => 'black'); - editorConfig.setup((p) => p.get('defaultFormatter', TypeMoq.It.isAny())).returns(() => BLACK_EXTENSION); - - await prompt.showInstallFormatterPrompt(); - assert.isTrue(isExtensionEnabledStub.notCalled); - }); - - test('Prompt: user selects do not show', async () => { - persistState.setup((p) => p.value).returns(() => false); - persistState - .setup((p) => p.updateValue(true)) - .returns(() => Promise.resolve()) - .verifiable(TypeMoq.Times.atLeastOnce()); - pythonConfig.setup((p) => p.get('formatting.provider', TypeMoq.It.isAny())).returns(() => 'autopep8'); - editorConfig.setup((p) => p.get('defaultFormatter', TypeMoq.It.isAny())).returns(() => ''); - isExtensionEnabledStub.returns(undefined); - - showInformationMessageStub.resolves(Common.doNotShowAgain); - - await prompt.showInstallFormatterPrompt(); - assert.isTrue( - showInformationMessageStub.calledWith( - ToolsExtensions.installAutopep8FormatterPrompt, - 'Black', - 'Autopep8', - Common.doNotShowAgain, - ), - 'showInformationMessage should be called', - ); - persistState.verifyAll(); - }); - - test('Prompt (autopep8): user selects Autopep8', async () => { - persistState.setup((p) => p.value).returns(() => false); - pythonConfig.setup((p) => p.get('formatting.provider', TypeMoq.It.isAny())).returns(() => 'autopep8'); - editorConfig.setup((p) => p.get('defaultFormatter', TypeMoq.It.isAny())).returns(() => ''); - isExtensionEnabledStub.returns(undefined); - - showInformationMessageStub.resolves('Autopep8'); - - await prompt.showInstallFormatterPrompt(); - assert.isTrue( - showInformationMessageStub.calledWith( - ToolsExtensions.installAutopep8FormatterPrompt, - 'Black', - 'Autopep8', - Common.doNotShowAgain, - ), - 'showInformationMessage should be called', - ); - assert.isTrue( - installFormatterExtensionStub.calledWith(AUTOPEP8_EXTENSION, undefined), - 'installFormatterExtension should be called', - ); - }); - - test('Prompt (autopep8): user selects Black', async () => { - persistState.setup((p) => p.value).returns(() => false); - pythonConfig.setup((p) => p.get('formatting.provider', TypeMoq.It.isAny())).returns(() => 'autopep8'); - editorConfig.setup((p) => p.get('defaultFormatter', TypeMoq.It.isAny())).returns(() => ''); - isExtensionEnabledStub.returns(undefined); - - showInformationMessageStub.resolves('Black'); - - await prompt.showInstallFormatterPrompt(); - assert.isTrue( - showInformationMessageStub.calledWith( - ToolsExtensions.installAutopep8FormatterPrompt, - 'Black', - 'Autopep8', - Common.doNotShowAgain, - ), - 'showInformationMessage should be called', - ); - assert.isTrue( - installFormatterExtensionStub.calledWith(BLACK_EXTENSION, undefined), - 'installFormatterExtension should be called', - ); - }); - - test('Prompt (black): user selects Autopep8', async () => { - persistState.setup((p) => p.value).returns(() => false); - pythonConfig.setup((p) => p.get('formatting.provider', TypeMoq.It.isAny())).returns(() => 'black'); - editorConfig.setup((p) => p.get('defaultFormatter', TypeMoq.It.isAny())).returns(() => ''); - isExtensionEnabledStub.returns(undefined); - - showInformationMessageStub.resolves('Autopep8'); - - await prompt.showInstallFormatterPrompt(); - assert.isTrue( - showInformationMessageStub.calledWith( - ToolsExtensions.installBlackFormatterPrompt, - 'Black', - 'Autopep8', - Common.doNotShowAgain, - ), - 'showInformationMessage should be called', - ); - assert.isTrue( - installFormatterExtensionStub.calledWith(AUTOPEP8_EXTENSION, undefined), - 'installFormatterExtension should be called', - ); - }); - - test('Prompt (black): user selects Black', async () => { - persistState.setup((p) => p.value).returns(() => false); - pythonConfig.setup((p) => p.get('formatting.provider', TypeMoq.It.isAny())).returns(() => 'black'); - editorConfig.setup((p) => p.get('defaultFormatter', TypeMoq.It.isAny())).returns(() => ''); - isExtensionEnabledStub.returns(undefined); - - showInformationMessageStub.resolves('Black'); - - await prompt.showInstallFormatterPrompt(); - assert.isTrue( - showInformationMessageStub.calledWith( - ToolsExtensions.installBlackFormatterPrompt, - 'Black', - 'Autopep8', - Common.doNotShowAgain, - ), - 'showInformationMessage should be called', - ); - assert.isTrue( - installFormatterExtensionStub.calledWith(BLACK_EXTENSION, undefined), - 'installFormatterExtension should be called', - ); - }); - - test('Prompt: Black and Autopep8 installed user selects Black as default', async () => { - persistState.setup((p) => p.value).returns(() => false); - pythonConfig.setup((p) => p.get('formatting.provider', TypeMoq.It.isAny())).returns(() => 'black'); - editorConfig.setup((p) => p.get('defaultFormatter', TypeMoq.It.isAny())).returns(() => ''); - isExtensionEnabledStub.returns({}); - - showInformationMessageStub.resolves('Black'); - - await prompt.showInstallFormatterPrompt(); - assert.isTrue( - showInformationMessageStub.calledWith( - ToolsExtensions.selectMultipleFormattersPrompt, - 'Black', - 'Autopep8', - Common.doNotShowAgain, - ), - 'showInformationMessage should be called', - ); - assert.isTrue( - updateDefaultFormatterStub.calledWith(BLACK_EXTENSION, undefined), - 'updateDefaultFormatter should be called', - ); - }); - - test('Prompt: Black and Autopep8 installed user selects Autopep8 as default', async () => { - persistState.setup((p) => p.value).returns(() => false); - pythonConfig.setup((p) => p.get('formatting.provider', TypeMoq.It.isAny())).returns(() => 'autopep8'); - editorConfig.setup((p) => p.get('defaultFormatter', TypeMoq.It.isAny())).returns(() => ''); - isExtensionEnabledStub.returns({}); - - showInformationMessageStub.resolves('Autopep8'); - - await prompt.showInstallFormatterPrompt(); - assert.isTrue( - showInformationMessageStub.calledWith( - ToolsExtensions.selectMultipleFormattersPrompt, - 'Black', - 'Autopep8', - Common.doNotShowAgain, - ), - 'showInformationMessage should be called', - ); - assert.isTrue( - updateDefaultFormatterStub.calledWith(AUTOPEP8_EXTENSION, undefined), - 'updateDefaultFormatter should be called', - ); - }); - - test('Prompt: Black installed user selects Black as default', async () => { - persistState.setup((p) => p.value).returns(() => false); - pythonConfig.setup((p) => p.get('formatting.provider', TypeMoq.It.isAny())).returns(() => 'black'); - editorConfig.setup((p) => p.get('defaultFormatter', TypeMoq.It.isAny())).returns(() => ''); - isExtensionEnabledStub.callsFake((extensionId) => { - if (extensionId === BLACK_EXTENSION) { - return {}; - } - return undefined; - }); - - showInformationMessageStub.resolves('Black'); - - await prompt.showInstallFormatterPrompt(); - assert.isTrue( - showInformationMessageStub.calledWith( - ToolsExtensions.selectBlackFormatterPrompt, - Common.bannerLabelYes, - Common.doNotShowAgain, - ), - 'showInformationMessage should be called', - ); - assert.isTrue( - updateDefaultFormatterStub.calledWith(BLACK_EXTENSION, undefined), - 'updateDefaultFormatter should be called', - ); - }); - - test('Prompt: Autopep8 installed user selects Autopep8 as default', async () => { - persistState.setup((p) => p.value).returns(() => false); - pythonConfig.setup((p) => p.get('formatting.provider', TypeMoq.It.isAny())).returns(() => 'autopep8'); - editorConfig.setup((p) => p.get('defaultFormatter', TypeMoq.It.isAny())).returns(() => ''); - isExtensionEnabledStub.callsFake((extensionId) => { - if (extensionId === AUTOPEP8_EXTENSION) { - return {}; - } - return undefined; - }); - - showInformationMessageStub.resolves('Autopep8'); - - await prompt.showInstallFormatterPrompt(); - assert.isTrue( - showInformationMessageStub.calledWith( - ToolsExtensions.selectAutopep8FormatterPrompt, - Common.bannerLabelYes, - Common.doNotShowAgain, - ), - 'showInformationMessage should be called', - ); - assert.isTrue( - updateDefaultFormatterStub.calledWith(AUTOPEP8_EXTENSION, undefined), - 'updateDefaultFormatter should be called', - ); - }); -}); From 582b21457668e8db4d1fd7ee5871e7cbdea3bc4d Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Wed, 11 Oct 2023 11:47:45 -0700 Subject: [PATCH 3/3] Update text in prompt --- src/client/logging/settingLogs.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/client/logging/settingLogs.ts b/src/client/logging/settingLogs.ts index f892dea99b76..721ab80d4500 100644 --- a/src/client/logging/settingLogs.ts +++ b/src/client/logging/settingLogs.ts @@ -21,7 +21,7 @@ export function logAndNotifyOnFormatterSetting(): void { const formatter = config.get('defaultFormatter', ''); traceInfo(`Default formatter is set to ${formatter} for workspace ${workspace.uri.fsPath}`); if (formatter === PVSC_EXTENSION_ID) { - traceError('Formatting feature is moved to separate extensions depending on the formatter you prefer.'); + traceError('Formatting features have been moved to separate formatter extensions.'); traceError('Please install the formatter extension you prefer and set it as the default formatter.'); traceError('For `autopep8` use: https://marketplace.visualstudio.com/items?itemName=ms-python.autopep8'); traceError( @@ -30,7 +30,7 @@ export function logAndNotifyOnFormatterSetting(): void { traceError('For `yapf` use: https://marketplace.visualstudio.com/items?itemName=eeyore.yapf'); const response = await showErrorMessage( l10n.t( - 'Formatting feature is moved to separate extensions depending on the formatter you prefer. Please install the formatter extension you prefer and set it as the default formatter.', + 'Formatting features have been moved to separate formatter extensions. Please install the formatter extension you prefer and set it as the default formatter.', ), Common.showLogs, );