Show a general warning prompt pointing to the upgrade guide when users attempt to run isort5 using deprecated settings

news/1 Enhancements/13716.md

@@ -0,0 +1 @@
+Show a general warning prompt pointing to the upgrade guide when users attempt to run isort5 using deprecated settings.

package.nls.json

@@ -306,6 +306,7 @@
     "diagnostics.invalidDebuggerTypeDiagnostic": "Your launch.json file needs to be updated to change the \"pythonExperimental\" debug configurations to use the \"python\" debugger type, otherwise Python debugging may not work. Would you like to automatically update your launch.json file now?",
     "diagnostics.consoleTypeDiagnostic": "Your launch.json file needs to be updated to change the console type string from \"none\" to \"internalConsole\", otherwise Python debugging may not work. Would you like to automatically update your launch.json file now?",
     "diagnostics.justMyCodeDiagnostic": "Configuration \"debugStdLib\" in launch.json is no longer supported. It's recommended to replace it with \"justMyCode\", which is the exact opposite of using \"debugStdLib\". Would you like to automatically update your launch.json file to do that?",
+    "diagnostics.checkIsort5UpgradeGuide": "We found outdated configuration for sorting imports in this workspace. Check the [isort upgrade guide](https://aka.ms/AA9j5x4) to update your settings.",
     "diagnostics.yesUpdateLaunch": "Yes, update launch.json",
     "diagnostics.invalidTestSettings": "Your settings needs to be updated to change the setting \"python.unitTest.\" to \"python.testing.\", otherwise testing Python code using the extension may not work. Would you like to automatically update your settings now?",
     "DataScience.interruptKernel": "Interrupt IPython kernel",

src/client/common/utils/localize.ts

@@ -60,6 +60,10 @@ export namespace Diagnostics {
         'Your settings needs to be updated to change the setting "python.unitTest." to "python.testing.", otherwise testing Python code using the extension may not work. Would you like to automatically update your settings now?'
     );
     export const updateSettings = localize('diagnostics.updateSettings', 'Yes, update settings');
+    export const checkIsort5UpgradeGuide = localize(
+        'diagnostics.checkIsort5UpgradeGuide',
+        'We found outdated configuration for sorting imports in this workspace. Check the [isort upgrade guide](https://aka.ms/AA9j5x4) to update your settings.'
+    );
 }
 
 export namespace Common {

src/client/providers/importSortProvider.ts

@@ -7,14 +7,23 @@ import { Commands, PYTHON_LANGUAGE, STANDARD_OUTPUT_CHANNEL } from '../common/co
 import { traceError } from '../common/logger';
 import * as internalScripts from '../common/process/internal/scripts';
 import { IProcessServiceFactory, IPythonExecutionFactory, ObservableExecutionResult } from '../common/process/types';
-import { IConfigurationService, IDisposableRegistry, IEditorUtils, IOutputChannel } from '../common/types';
+import {
+    IConfigurationService,
+    IDisposableRegistry,
+    IEditorUtils,
+    IOutputChannel,
+    IPersistentStateFactory
+} from '../common/types';
 import { createDeferred, createDeferredFromPromise, Deferred } from '../common/utils/async';
+import { Common, Diagnostics } from '../common/utils/localize';
 import { noop } from '../common/utils/misc';
 import { IServiceContainer } from '../ioc/types';
 import { captureTelemetry } from '../telemetry';
 import { EventName } from '../telemetry/constants';
 import { ISortImportsEditingProvider } from './types';
 
+const doNotDisplayPromptStateKey = 'ISORT5_UPGRADE_WARNING_KEY';
+
 @injectable()
 export class SortImportsEditingProvider implements ISortImportsEditingProvider {
     private readonly isortPromises = new Map<
@@ -24,9 +33,11 @@ export class SortImportsEditingProvider implements ISortImportsEditingProvider {
     private readonly processServiceFactory: IProcessServiceFactory;
     private readonly pythonExecutionFactory: IPythonExecutionFactory;
     private readonly shell: IApplicationShell;
+    private readonly persistentStateFactory: IPersistentStateFactory;
     private readonly documentManager: IDocumentManager;
     private readonly configurationService: IConfigurationService;
     private readonly editorUtils: IEditorUtils;
+    private readonly output: IOutputChannel;
 
     public constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) {
         this.shell = serviceContainer.get<IApplicationShell>(IApplicationShell);
@@ -35,6 +46,8 @@ export class SortImportsEditingProvider implements ISortImportsEditingProvider {
         this.pythonExecutionFactory = serviceContainer.get<IPythonExecutionFactory>(IPythonExecutionFactory);
         this.processServiceFactory = serviceContainer.get<IProcessServiceFactory>(IProcessServiceFactory);
         this.editorUtils = serviceContainer.get<IEditorUtils>(IEditorUtils);
+        this.output = serviceContainer.get<IOutputChannel>(IOutputChannel, STANDARD_OUTPUT_CHANNEL);
+        this.persistentStateFactory = serviceContainer.get<IPersistentStateFactory>(IPersistentStateFactory);
     }
 
     @captureTelemetry(EventName.FORMAT_SORT_IMPORTS)
@@ -122,6 +135,26 @@ export class SortImportsEditingProvider implements ISortImportsEditingProvider {
         }
     }
 
+    public async _showWarningAndOptionallyShowOutput() {
+        const neverShowAgain = this.persistentStateFactory.createGlobalPersistentState(
+            doNotDisplayPromptStateKey,
+            false
+        );
+        if (neverShowAgain.value) {
+            return;
+        }
+        const selection = await this.shell.showWarningMessage(
+            Diagnostics.checkIsort5UpgradeGuide(),
+            Common.openOutputPanel(),
+            Common.doNotShowAgain()
+        );
+        if (selection === Common.openOutputPanel()) {
+            this.output.show(true);
+        } else if (selection === Common.doNotShowAgain()) {
+            await neverShowAgain.updateValue(true);
+        }
+    }
+
     private async getExecIsort(
         document: TextDocument,
         uri: Uri,
@@ -141,7 +174,6 @@ export class SortImportsEditingProvider implements ISortImportsEditingProvider {
 
         const spawnOptions = {
             token,
-            throwOnStdErr: true,
             cwd: path.dirname(uri.fsPath)
         };
 
@@ -169,11 +201,20 @@ export class SortImportsEditingProvider implements ISortImportsEditingProvider {
     ): Promise<string> {
         // Configure our listening to the output from isort ...
         let outputBuffer = '';
+        let isAnyErrorRelatedToUpgradeGuide = false;
         const isortOutput = createDeferred<string>();
         observableResult.out.subscribe({
             next: (output) => {
                 if (output.source === 'stdout') {
                     outputBuffer += output.out;
+                } else {
+                    // All the W0500 warning codes point to isort5 upgrade guide: https://pycqa.github.io/isort/docs/warning_and_error_codes/W0500/
+                    // Do not throw error on these types of stdErrors
+                    isAnyErrorRelatedToUpgradeGuide = isAnyErrorRelatedToUpgradeGuide || output.out.includes('W050');
+                    traceError(output.out);
+                    if (!output.out.includes('W050')) {
+                        isortOutput.reject(output.out);
+                    }
                 }
             },
             complete: () => {
@@ -188,6 +229,9 @@ export class SortImportsEditingProvider implements ISortImportsEditingProvider {
         // .. and finally wait for isort to do its thing
         await isortOutput.promise;
 
+        if (isAnyErrorRelatedToUpgradeGuide) {
+            this._showWarningAndOptionallyShowOutput().ignoreErrors();
+        }
         return outputBuffer;
     }
 }

src/test/providers/importSortProvider.unit.test.ts

@@ -5,17 +5,18 @@
 
 // tslint:disable:no-any max-func-body-length
 
-import { expect } from 'chai';
+import { assert, expect } from 'chai';
 import { ChildProcess } from 'child_process';
 import { EOL } from 'os';
 import * as path from 'path';
 import { Observable } from 'rxjs/Observable';
 import { Subscriber } from 'rxjs/Subscriber';
+import * as sinon from 'sinon';
 import { Writable } from 'stream';
 import * as TypeMoq from 'typemoq';
 import { Range, TextDocument, TextEditor, TextLine, Uri, WorkspaceEdit } from 'vscode';
 import { IApplicationShell, ICommandManager, IDocumentManager } from '../../client/common/application/types';
-import { Commands, EXTENSION_ROOT_DIR } from '../../client/common/constants';
+import { Commands, EXTENSION_ROOT_DIR, STANDARD_OUTPUT_CHANNEL } from '../../client/common/constants';
 import { ProcessService } from '../../client/common/process/proc';
 import {
     IProcessServiceFactory,
@@ -27,10 +28,14 @@ import {
     IConfigurationService,
     IDisposableRegistry,
     IEditorUtils,
+    IOutputChannel,
+    IPersistentState,
+    IPersistentStateFactory,
     IPythonSettings,
     ISortImportSettings
 } from '../../client/common/types';
 import { createDeferred, createDeferredFromPromise } from '../../client/common/utils/async';
+import { Common, Diagnostics } from '../../client/common/utils/localize';
 import { noop } from '../../client/common/utils/misc';
 import { IServiceContainer } from '../../client/ioc/types';
 import { SortImportsEditingProvider } from '../../client/providers/importSortProvider';
@@ -48,6 +53,8 @@ suite('Import Sort Provider', () => {
     let editorUtils: TypeMoq.IMock<IEditorUtils>;
     let commandManager: TypeMoq.IMock<ICommandManager>;
     let pythonSettings: TypeMoq.IMock<IPythonSettings>;
+    let persistentStateFactory: TypeMoq.IMock<IPersistentStateFactory>;
+    let output: TypeMoq.IMock<IOutputChannel>;
     let sortProvider: SortImportsEditingProvider;
     setup(() => {
         serviceContainer = TypeMoq.Mock.ofType<IServiceContainer>();
@@ -59,6 +66,10 @@ suite('Import Sort Provider', () => {
         processServiceFactory = TypeMoq.Mock.ofType<IProcessServiceFactory>();
         pythonSettings = TypeMoq.Mock.ofType<IPythonSettings>();
         editorUtils = TypeMoq.Mock.ofType<IEditorUtils>();
+        persistentStateFactory = TypeMoq.Mock.ofType<IPersistentStateFactory>();
+        output = TypeMoq.Mock.ofType<IOutputChannel>();
+        serviceContainer.setup((c) => c.get(IOutputChannel, STANDARD_OUTPUT_CHANNEL)).returns(() => output.object);
+        serviceContainer.setup((c) => c.get(IPersistentStateFactory)).returns(() => persistentStateFactory.object);
         serviceContainer.setup((c) => c.get(ICommandManager)).returns(() => commandManager.object);
         serviceContainer.setup((c) => c.get(IDocumentManager)).returns(() => documentManager.object);
         serviceContainer.setup((c) => c.get(IApplicationShell)).returns(() => shell.object);
@@ -71,6 +82,10 @@ suite('Import Sort Provider', () => {
         sortProvider = new SortImportsEditingProvider(serviceContainer.object);
     });
 
+    teardown(() => {
+        sinon.restore();
+    });
+
     test('Ensure command is registered', () => {
         commandManager
             .setup((c) =>
@@ -341,7 +356,7 @@ suite('Import Sort Provider', () => {
                 p.execObservable(
                     TypeMoq.It.isValue('CUSTOM_ISORT'),
                     TypeMoq.It.isValue(expectedArgs),
-                    TypeMoq.It.isValue({ throwOnStdErr: true, token: undefined, cwd: path.sep })
+                    TypeMoq.It.isValue({ token: undefined, cwd: path.sep })
                 )
             )
             .returns(() => executionResult)
@@ -428,7 +443,7 @@ suite('Import Sort Provider', () => {
             .setup((p) =>
                 p.execObservable(
                     TypeMoq.It.isValue(expectedArgs),
-                    TypeMoq.It.isValue({ throwOnStdErr: true, token: undefined, cwd: path.sep })
+                    TypeMoq.It.isValue({ token: undefined, cwd: path.sep })
                 )
             )
             .returns(() => executionResult)
@@ -556,4 +571,126 @@ suite('Import Sort Provider', () => {
         expect(edit).to.be.equal(undefined, 'The results from the first execution should be discarded');
         stdinStream1.verifyAll();
     });
+
+    test('If isort raises a warning message related to isort5 upgrade guide, show message', async () => {
+        const _showWarningAndOptionallyShowOutput = sinon.stub(
+            SortImportsEditingProvider.prototype,
+            '_showWarningAndOptionallyShowOutput'
+        );
+        _showWarningAndOptionallyShowOutput.resolves();
+        const uri = Uri.file('something.py');
+        const mockDoc = TypeMoq.Mock.ofType<TextDocument>();
+        const processService = TypeMoq.Mock.ofType<ProcessService>();
+        processService.setup((d: any) => d.then).returns(() => undefined);
+        mockDoc.setup((d: any) => d.then).returns(() => undefined);
+        mockDoc.setup((d) => d.lineCount).returns(() => 10);
+        mockDoc.setup((d) => d.getText(TypeMoq.It.isAny())).returns(() => 'Hello');
+        mockDoc.setup((d) => d.isDirty).returns(() => true);
+        mockDoc.setup((d) => d.uri).returns(() => uri);
+        documentManager
+            .setup((d) => d.openTextDocument(TypeMoq.It.isValue(uri)))
+            .returns(() => Promise.resolve(mockDoc.object));
+        pythonSettings
+            .setup((s) => s.sortImports)
+            .returns(() => {
+                return ({ path: 'CUSTOM_ISORT', args: [] } as any) as ISortImportSettings;
+            });
+        processServiceFactory
+            .setup((p) => p.create(TypeMoq.It.isAny()))
+            .returns(() => Promise.resolve(processService.object));
+        const result = new WorkspaceEdit();
+        editorUtils
+            .setup((e) =>
+                e.getWorkspaceEditsFromPatch(
+                    TypeMoq.It.isValue('Hello'),
+                    TypeMoq.It.isValue('DIFF'),
+                    TypeMoq.It.isAny()
+                )
+            )
+            .returns(() => result);
+
+        // ----------------------Run the command----------------------
+        let subscriber: Subscriber<Output<string>>;
+        const stdinStream = TypeMoq.Mock.ofType<Writable>();
+        stdinStream.setup((s) => s.write('Hello'));
+        stdinStream
+            .setup((s) => s.end())
+            .callback(() => {
+                subscriber.next({ source: 'stdout', out: 'DIFF' });
+                subscriber.next({ source: 'stderr', out: 'Some warning related to isort5 (W0503)' });
+                subscriber.complete();
+            })
+            .verifiable(TypeMoq.Times.once());
+        const childProcess = TypeMoq.Mock.ofType<ChildProcess>();
+        childProcess.setup((p) => p.stdin).returns(() => stdinStream.object);
+        const executionResult = {
+            proc: childProcess.object,
+            out: new Observable<Output<string>>((s) => (subscriber = s)),
+            dispose: noop
+        };
+        processService.reset();
+        processService.setup((d: any) => d.then).returns(() => undefined);
+        processService
+            .setup((p) => p.execObservable(TypeMoq.It.isValue('CUSTOM_ISORT'), TypeMoq.It.isAny(), TypeMoq.It.isAny()))
+            .returns(() => executionResult);
+
+        const edit = await sortProvider.provideDocumentSortImportsEdits(uri);
+
+        // ----------------------Verify results----------------------
+        expect(edit).to.be.equal(result, 'Execution result is incorrect');
+        assert.ok(_showWarningAndOptionallyShowOutput.calledOnce);
+        stdinStream.verifyAll();
+    });
+
+    test('If user clicks show output on the isort5 warning prompt, show the Python output', async () => {
+        const neverShowAgain = TypeMoq.Mock.ofType<IPersistentState<boolean>>();
+        persistentStateFactory
+            .setup((p) => p.createGlobalPersistentState(TypeMoq.It.isAny(), false))
+            .returns(() => neverShowAgain.object);
+        neverShowAgain.setup((p) => p.value).returns(() => false);
+        shell
+            .setup((s) =>
+                s.showWarningMessage(
+                    Diagnostics.checkIsort5UpgradeGuide(),
+                    Common.openOutputPanel(),
+                    Common.doNotShowAgain()
+                )
+            )
+            .returns(() => Promise.resolve(Common.openOutputPanel()));
+        output.setup((o) => o.show(true)).verifiable(TypeMoq.Times.once());
+        await sortProvider._showWarningAndOptionallyShowOutput();
+        output.verifyAll();
+    });
+
+    test('If user clicks do not show again on the isort5 warning prompt, do not show the prompt again', async () => {
+        const neverShowAgain = TypeMoq.Mock.ofType<IPersistentState<boolean>>();
+        persistentStateFactory
+            .setup((p) => p.createGlobalPersistentState(TypeMoq.It.isAny(), false))
+            .returns(() => neverShowAgain.object);
+        let doNotShowAgainValue = false;
+        neverShowAgain.setup((p) => p.value).returns(() => doNotShowAgainValue);
+        neverShowAgain
+            .setup((p) => p.updateValue(true))
+            .returns(() => {
+                doNotShowAgainValue = true;
+                return Promise.resolve();
+            });
+        shell
+            .setup((s) =>
+                s.showWarningMessage(
+                    Diagnostics.checkIsort5UpgradeGuide(),
+                    Common.openOutputPanel(),
+                    Common.doNotShowAgain()
+                )
+            )
+            .returns(() => Promise.resolve(Common.doNotShowAgain()))
+            .verifiable(TypeMoq.Times.once());
+
+        await sortProvider._showWarningAndOptionallyShowOutput();
+        shell.verifyAll();
+
+        await sortProvider._showWarningAndOptionallyShowOutput();
+        await sortProvider._showWarningAndOptionallyShowOutput();
+        shell.verifyAll();
+    });
 });