From a5d5d42f19c54cf9cb2527a1fff391775c5083eb Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Wed, 6 May 2020 19:24:53 -0700 Subject: [PATCH 1/3] Show the prompt again if user clicks on more info --- .../checks/pythonPathDeprecated.ts | 15 +++- .../checks/pythonPathDeprecated.unit.test.ts | 79 +++++++++++++++---- 2 files changed, 76 insertions(+), 18 deletions(-) diff --git a/src/client/application/diagnostics/checks/pythonPathDeprecated.ts b/src/client/application/diagnostics/checks/pythonPathDeprecated.ts index 6ba46dd11042..3960825505d2 100644 --- a/src/client/application/diagnostics/checks/pythonPathDeprecated.ts +++ b/src/client/application/diagnostics/checks/pythonPathDeprecated.ts @@ -99,10 +99,17 @@ export class PythonPathDeprecatedDiagnosticService extends BaseDiagnosticsServic }, { prompt: Common.moreInfo(), - command: commandFactory.createCommand(diagnostic, { - type: 'launch', - options: learnMoreOnInterpreterSecurityURI - }) + command: { + diagnostic, + invoke: async (): Promise => { + const launchCommand = commandFactory.createCommand(diagnostic, { + type: 'launch', + options: learnMoreOnInterpreterSecurityURI + }); + await launchCommand.invoke(); + return this.messageService.handle(diagnostic, { commandPrompts: options }); + } + } }, { prompt: Common.doNotShowAgain(), diff --git a/src/test/application/diagnostics/checks/pythonPathDeprecated.unit.test.ts b/src/test/application/diagnostics/checks/pythonPathDeprecated.unit.test.ts index 05bfaa95d6a7..4c675e3d0d74 100644 --- a/src/test/application/diagnostics/checks/pythonPathDeprecated.unit.test.ts +++ b/src/test/application/diagnostics/checks/pythonPathDeprecated.unit.test.ts @@ -142,7 +142,6 @@ suite('Application Diagnostics - Python Path Deprecated', () => { }); test('Python Path Deprecated Diagnostic is handled as expected', async () => { const diagnostic = new PythonPathDeprecatedDiagnostic('message', resource); - const launchCmd = ({ cmd: 'launchCmd' } as any) as IDiagnosticCommand; const ignoreCmd = ({ cmd: 'ignoreCmd' } as any) as IDiagnosticCommand; filterService .setup((f) => @@ -155,15 +154,6 @@ suite('Application Diagnostics - Python Path Deprecated', () => { .callback((_d, p: MessageCommandPrompt) => (messagePrompt = p)) .returns(() => Promise.resolve()) .verifiable(typemoq.Times.once()); - commandFactory - .setup((f) => - f.createCommand( - typemoq.It.isAny(), - typemoq.It.isObjectWith>({ type: 'launch' }) - ) - ) - .returns(() => launchCmd) - .verifiable(typemoq.Times.once()); commandFactory .setup((f) => @@ -185,10 +175,8 @@ suite('Application Diagnostics - Python Path Deprecated', () => { expect(messagePrompt!.commandPrompts[0].command!.diagnostic).to.be.deep.equal(diagnostic); expect(messagePrompt!.commandPrompts[0].prompt).to.be.deep.equal(Common.yesPlease()); expect(messagePrompt!.commandPrompts[1]).to.be.deep.equal({ prompt: Common.noIWillDoItLater() }); - expect(messagePrompt!.commandPrompts[2]).to.be.deep.equal({ - prompt: Common.moreInfo(), - command: launchCmd - }); + expect(messagePrompt!.commandPrompts[2].prompt).to.be.deep.equal(Common.moreInfo()); + expect(messagePrompt!.commandPrompts[2].command!.diagnostic).to.be.deep.equal(diagnostic); expect(messagePrompt!.commandPrompts[3]).to.be.deep.equal({ prompt: Common.doNotShowAgain(), command: ignoreCmd @@ -202,6 +190,69 @@ suite('Application Diagnostics - Python Path Deprecated', () => { await messagePrompt!.commandPrompts[0].command!.invoke(); assert(_removePythonPathFromWorkspaceSettings.calledOnceWith(resource)); }); + test('More info should show the prompt again', async () => { + const diagnostic = new PythonPathDeprecatedDiagnostic('message', resource); + const launchCmd: typemoq.IMock = typemoq.Mock.ofType(); + const ignoreCmd = ({ cmd: 'ignoreCmd' } as any) as IDiagnosticCommand; + + launchCmd + .setup((f) => f.invoke()) + .returns(() => Promise.resolve()) + .verifiable(typemoq.Times.once()); + filterService + .setup((f) => + f.shouldIgnoreDiagnostic(typemoq.It.isValue(DiagnosticCodes.PythonPathDeprecatedDiagnostic)) + ) + .returns(() => Promise.resolve(false)); + + // This will be invoked twice, first time to show the message + // Next time when more info is clicked. + let messagePrompt: MessageCommandPrompt | undefined; + messageHandler + .setup((i) => i.handle(typemoq.It.isValue(diagnostic), typemoq.It.isAny())) + .callback((_d, p: MessageCommandPrompt) => (messagePrompt = p)) + .returns(() => Promise.resolve()) + .verifiable(typemoq.Times.exactly(2)); + + commandFactory + .setup((f) => + f.createCommand( + typemoq.It.isAny(), + typemoq.It.isObjectWith>({ type: 'launch' }) + ) + ) + .returns(() => launchCmd.object) + .verifiable(typemoq.Times.once()); + + commandFactory + .setup((f) => + f.createCommand( + typemoq.It.isAny(), + typemoq.It.isObjectWith>({ type: 'ignore' }) + ) + ) + .returns(() => ignoreCmd) + .verifiable(typemoq.Times.once()); + + await diagnosticService.handle([diagnostic]); + + expect(messagePrompt).not.be.equal(undefined, 'Message prompt not set'); + expect(messagePrompt!.commandPrompts.length).to.equal(4, 'Incorrect length'); + expect(messagePrompt!.commandPrompts[0].command).not.be.equal(undefined, 'Command not set'); + expect(messagePrompt!.commandPrompts[0].command!.diagnostic).to.be.deep.equal(diagnostic); + expect(messagePrompt!.commandPrompts[0].prompt).to.be.deep.equal(Common.yesPlease()); + expect(messagePrompt!.commandPrompts[1]).to.be.deep.equal({ prompt: Common.noIWillDoItLater() }); + expect(messagePrompt!.commandPrompts[2].prompt).to.be.deep.equal(Common.moreInfo()); + expect(messagePrompt!.commandPrompts[2].command!.diagnostic).to.be.deep.equal(diagnostic); + expect(messagePrompt!.commandPrompts[3]).to.be.deep.equal({ + prompt: Common.doNotShowAgain(), + command: ignoreCmd + }); + + await messagePrompt!.commandPrompts[2].command!.invoke(); + messageHandler.verifyAll(); + commandFactory.verifyAll(); + }); test('Handling an empty diagnostic should not show a message nor return a command', async () => { const diagnostics: IDiagnostic[] = []; From 39998e34c94787b2dd3fee33248868ad025ebc68 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Thu, 7 May 2020 12:08:38 -0700 Subject: [PATCH 2/3] Review feedback --- package.nls.json | 6 +- .../checks/pythonPathDeprecated.ts | 15 ---- src/client/common/utils/localize.ts | 6 +- .../checks/pythonPathDeprecated.unit.test.ts | 69 +------------------ 4 files changed, 8 insertions(+), 88 deletions(-) diff --git a/package.nls.json b/package.nls.json index 40e60ba8e39f..e8f4777e4f22 100644 --- a/package.nls.json +++ b/package.nls.json @@ -240,9 +240,9 @@ "DataScience.liveShareServiceFailure": "Failure starting '{0}' service during live share connection.", "DataScience.documentMismatch": "Cannot run cells, duplicate documents for {0} found.", "DataScience.pythonInteractiveCreateFailed": "Failure to create a 'Python Interactive' window. Try reinstalling the Python extension.", - "diagnostics.removePythonPathSettingsJson": "The setting \"python.pythonPath\" defined in your workspace settings is now deprecated. Do you want us to delete it? This will only remove the \"python.pythonPath\" entry from your settings.json.", - "diagnostics.removePythonPathCodeWorkspace": "The setting \"python.pythonPath\" defined in your workspace settings is now deprecated. Do you want us to delete it? This will only remove the \"python.pythonPath\" entry from your .code-workspace file.", - "diagnostics.removePythonPathCodeWorkspaceAndSettingsJson": "The setting \"python.pythonPath\" defined in your workspace settings is now deprecated. Do you want us to delete it? This will only remove the \"python.pythonPath\" entry from your settings.json and .code-workspace file.", + "diagnostics.removePythonPathSettingsJson": "The setting \"python.pythonPath\" defined in your settings.json is now deprecated. Do you want us to delete it from your settings.json only? Refer to [this documentation](https://aka.ms/AA7jfor) to learn more.", + "diagnostics.removePythonPathCodeWorkspace": "The setting \"python.pythonPath\" defined in your workspace settings is now deprecated. Do you want us to delete it from your .code-workspace file only? Refer to [this documentation](https://aka.ms/AA7jfor) to learn more.", + "diagnostics.removePythonPathCodeWorkspaceAndSettingsJson": "The setting \"python.pythonPath\" defined in your workspace settings is now deprecated. Do you want us to delete it from your .code-workspace file and settings.json? Refer to [this documentation](https://aka.ms/AA7jfor) to learn more.", "diagnostics.warnSourceMaps": "Source map support is enabled in the Python Extension, this will adversely impact performance of the extension.", "diagnostics.disableSourceMaps": "Disable Source Map Support", "diagnostics.warnBeforeEnablingSourceMaps": "Enabling source map support in the Python Extension will adversely impact performance of the extension.", diff --git a/src/client/application/diagnostics/checks/pythonPathDeprecated.ts b/src/client/application/diagnostics/checks/pythonPathDeprecated.ts index 3960825505d2..80dd0d5b0cbc 100644 --- a/src/client/application/diagnostics/checks/pythonPathDeprecated.ts +++ b/src/client/application/diagnostics/checks/pythonPathDeprecated.ts @@ -9,7 +9,6 @@ import { IWorkspaceService } from '../../../common/application/types'; import { DeprecatePythonPath } from '../../../common/experimentGroups'; import { IDisposableRegistry, IExperimentsManager, Resource } from '../../../common/types'; import { Common, Diagnostics } from '../../../common/utils/localize'; -import { learnMoreOnInterpreterSecurityURI } from '../../../interpreter/autoSelection/constants'; import { IServiceContainer } from '../../../ioc/types'; import { BaseDiagnostic, BaseDiagnosticsService } from '../base'; import { IDiagnosticsCommandFactory } from '../commands/types'; @@ -97,20 +96,6 @@ export class PythonPathDeprecatedDiagnosticService extends BaseDiagnosticsServic { prompt: Common.noIWillDoItLater() }, - { - prompt: Common.moreInfo(), - command: { - diagnostic, - invoke: async (): Promise => { - const launchCommand = commandFactory.createCommand(diagnostic, { - type: 'launch', - options: learnMoreOnInterpreterSecurityURI - }); - await launchCommand.invoke(); - return this.messageService.handle(diagnostic, { commandPrompts: options }); - } - } - }, { prompt: Common.doNotShowAgain(), command: commandFactory.createCommand(diagnostic, { type: 'ignore', options: DiagnosticScope.Global }) diff --git a/src/client/common/utils/localize.ts b/src/client/common/utils/localize.ts index 37c84003fc09..86185f92ed8c 100644 --- a/src/client/common/utils/localize.ts +++ b/src/client/common/utils/localize.ts @@ -32,15 +32,15 @@ export namespace Diagnostics { ); export const removePythonPathSettingsJson = localize( 'diagnostics.removePythonPathSettingsJson', - 'The setting "python.pythonPath" defined in your workspace settings is now deprecated. Do you want us to delete it? This will only remove the "python.pythonPath" entry from your settings.json.' + 'The setting "python.pythonPath" defined in your settings.json is now deprecated. Do you want us to delete it from your settings.json only? Refer to [this documentation](https://aka.ms/AA7jfor) to learn more.' ); export const removePythonPathCodeWorkspace = localize( 'diagnostics.removePythonPathCodeWorkspace', - 'The setting "python.pythonPath" defined in your workspace settings is now deprecated. Do you want us to delete it? This will only remove the "python.pythonPath" entry from your .code-workspace file.' + 'The setting "python.pythonPath" defined in your workspace settings is now deprecated. Do you want us to delete it from your .code-workspace file only? Refer to [this documentation](https://aka.ms/AA7jfor) to learn more.' ); export const removePythonPathCodeWorkspaceAndSettingsJson = localize( 'diagnostics.removePythonPathCodeWorkspaceAndSettingsJson', - 'The setting "python.pythonPath" defined in your workspace settings is now deprecated. Do you want us to delete it? This will only remove the "python.pythonPath" entry from your settings.json and .code-workspace file.' + 'The setting "python.pythonPath" defined in your workspace settings is now deprecated. Do you want us to delete it from your .code-workspace file and settings.json? Refer to [this documentation](https://aka.ms/AA7jfor) to learn more.' ); export const invalidPythonPathInDebuggerSettings = localize( 'diagnostics.invalidPythonPathInDebuggerSettings', diff --git a/src/test/application/diagnostics/checks/pythonPathDeprecated.unit.test.ts b/src/test/application/diagnostics/checks/pythonPathDeprecated.unit.test.ts index 4c675e3d0d74..014bf92eff1e 100644 --- a/src/test/application/diagnostics/checks/pythonPathDeprecated.unit.test.ts +++ b/src/test/application/diagnostics/checks/pythonPathDeprecated.unit.test.ts @@ -170,14 +170,12 @@ suite('Application Diagnostics - Python Path Deprecated', () => { messageHandler.verifyAll(); commandFactory.verifyAll(); expect(messagePrompt).not.be.equal(undefined, 'Message prompt not set'); - expect(messagePrompt!.commandPrompts.length).to.equal(4, 'Incorrect length'); + expect(messagePrompt!.commandPrompts.length).to.equal(3, 'Incorrect length'); expect(messagePrompt!.commandPrompts[0].command).not.be.equal(undefined, 'Command not set'); expect(messagePrompt!.commandPrompts[0].command!.diagnostic).to.be.deep.equal(diagnostic); expect(messagePrompt!.commandPrompts[0].prompt).to.be.deep.equal(Common.yesPlease()); expect(messagePrompt!.commandPrompts[1]).to.be.deep.equal({ prompt: Common.noIWillDoItLater() }); - expect(messagePrompt!.commandPrompts[2].prompt).to.be.deep.equal(Common.moreInfo()); - expect(messagePrompt!.commandPrompts[2].command!.diagnostic).to.be.deep.equal(diagnostic); - expect(messagePrompt!.commandPrompts[3]).to.be.deep.equal({ + expect(messagePrompt!.commandPrompts[2]).to.be.deep.equal({ prompt: Common.doNotShowAgain(), command: ignoreCmd }); @@ -190,69 +188,6 @@ suite('Application Diagnostics - Python Path Deprecated', () => { await messagePrompt!.commandPrompts[0].command!.invoke(); assert(_removePythonPathFromWorkspaceSettings.calledOnceWith(resource)); }); - test('More info should show the prompt again', async () => { - const diagnostic = new PythonPathDeprecatedDiagnostic('message', resource); - const launchCmd: typemoq.IMock = typemoq.Mock.ofType(); - const ignoreCmd = ({ cmd: 'ignoreCmd' } as any) as IDiagnosticCommand; - - launchCmd - .setup((f) => f.invoke()) - .returns(() => Promise.resolve()) - .verifiable(typemoq.Times.once()); - filterService - .setup((f) => - f.shouldIgnoreDiagnostic(typemoq.It.isValue(DiagnosticCodes.PythonPathDeprecatedDiagnostic)) - ) - .returns(() => Promise.resolve(false)); - - // This will be invoked twice, first time to show the message - // Next time when more info is clicked. - let messagePrompt: MessageCommandPrompt | undefined; - messageHandler - .setup((i) => i.handle(typemoq.It.isValue(diagnostic), typemoq.It.isAny())) - .callback((_d, p: MessageCommandPrompt) => (messagePrompt = p)) - .returns(() => Promise.resolve()) - .verifiable(typemoq.Times.exactly(2)); - - commandFactory - .setup((f) => - f.createCommand( - typemoq.It.isAny(), - typemoq.It.isObjectWith>({ type: 'launch' }) - ) - ) - .returns(() => launchCmd.object) - .verifiable(typemoq.Times.once()); - - commandFactory - .setup((f) => - f.createCommand( - typemoq.It.isAny(), - typemoq.It.isObjectWith>({ type: 'ignore' }) - ) - ) - .returns(() => ignoreCmd) - .verifiable(typemoq.Times.once()); - - await diagnosticService.handle([diagnostic]); - - expect(messagePrompt).not.be.equal(undefined, 'Message prompt not set'); - expect(messagePrompt!.commandPrompts.length).to.equal(4, 'Incorrect length'); - expect(messagePrompt!.commandPrompts[0].command).not.be.equal(undefined, 'Command not set'); - expect(messagePrompt!.commandPrompts[0].command!.diagnostic).to.be.deep.equal(diagnostic); - expect(messagePrompt!.commandPrompts[0].prompt).to.be.deep.equal(Common.yesPlease()); - expect(messagePrompt!.commandPrompts[1]).to.be.deep.equal({ prompt: Common.noIWillDoItLater() }); - expect(messagePrompt!.commandPrompts[2].prompt).to.be.deep.equal(Common.moreInfo()); - expect(messagePrompt!.commandPrompts[2].command!.diagnostic).to.be.deep.equal(diagnostic); - expect(messagePrompt!.commandPrompts[3]).to.be.deep.equal({ - prompt: Common.doNotShowAgain(), - command: ignoreCmd - }); - - await messagePrompt!.commandPrompts[2].command!.invoke(); - messageHandler.verifyAll(); - commandFactory.verifyAll(); - }); test('Handling an empty diagnostic should not show a message nor return a command', async () => { const diagnostics: IDiagnostic[] = []; From 568c5d2f4fe3cbecba3160655d9c55e03e4e9ee3 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Thu, 7 May 2020 12:41:29 -0700 Subject: [PATCH 3/3] Use Learn more as text for the link. --- package.nls.json | 6 +++--- src/client/common/utils/localize.ts | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/package.nls.json b/package.nls.json index e8f4777e4f22..ff4f684fcaf1 100644 --- a/package.nls.json +++ b/package.nls.json @@ -240,9 +240,9 @@ "DataScience.liveShareServiceFailure": "Failure starting '{0}' service during live share connection.", "DataScience.documentMismatch": "Cannot run cells, duplicate documents for {0} found.", "DataScience.pythonInteractiveCreateFailed": "Failure to create a 'Python Interactive' window. Try reinstalling the Python extension.", - "diagnostics.removePythonPathSettingsJson": "The setting \"python.pythonPath\" defined in your settings.json is now deprecated. Do you want us to delete it from your settings.json only? Refer to [this documentation](https://aka.ms/AA7jfor) to learn more.", - "diagnostics.removePythonPathCodeWorkspace": "The setting \"python.pythonPath\" defined in your workspace settings is now deprecated. Do you want us to delete it from your .code-workspace file only? Refer to [this documentation](https://aka.ms/AA7jfor) to learn more.", - "diagnostics.removePythonPathCodeWorkspaceAndSettingsJson": "The setting \"python.pythonPath\" defined in your workspace settings is now deprecated. Do you want us to delete it from your .code-workspace file and settings.json? Refer to [this documentation](https://aka.ms/AA7jfor) to learn more.", + "diagnostics.removePythonPathSettingsJson": "The setting \"python.pythonPath\" defined in your settings.json is now deprecated. Do you want us to delete it from your settings.json only? [Learn more](https://aka.ms/AA7jfor).", + "diagnostics.removePythonPathCodeWorkspace": "The setting \"python.pythonPath\" defined in your workspace settings is now deprecated. Do you want us to delete it from your .code-workspace file only? [Learn more](https://aka.ms/AA7jfor).", + "diagnostics.removePythonPathCodeWorkspaceAndSettingsJson": "The setting \"python.pythonPath\" defined in your workspace settings is now deprecated. Do you want us to delete it from your .code-workspace file and settings.json? [Learn more](https://aka.ms/AA7jfor).", "diagnostics.warnSourceMaps": "Source map support is enabled in the Python Extension, this will adversely impact performance of the extension.", "diagnostics.disableSourceMaps": "Disable Source Map Support", "diagnostics.warnBeforeEnablingSourceMaps": "Enabling source map support in the Python Extension will adversely impact performance of the extension.", diff --git a/src/client/common/utils/localize.ts b/src/client/common/utils/localize.ts index 86185f92ed8c..04b952ff57dd 100644 --- a/src/client/common/utils/localize.ts +++ b/src/client/common/utils/localize.ts @@ -32,15 +32,15 @@ export namespace Diagnostics { ); export const removePythonPathSettingsJson = localize( 'diagnostics.removePythonPathSettingsJson', - 'The setting "python.pythonPath" defined in your settings.json is now deprecated. Do you want us to delete it from your settings.json only? Refer to [this documentation](https://aka.ms/AA7jfor) to learn more.' + 'The setting "python.pythonPath" defined in your settings.json is now deprecated. Do you want us to delete it from your settings.json only? [Learn more](https://aka.ms/AA7jfor).' ); export const removePythonPathCodeWorkspace = localize( 'diagnostics.removePythonPathCodeWorkspace', - 'The setting "python.pythonPath" defined in your workspace settings is now deprecated. Do you want us to delete it from your .code-workspace file only? Refer to [this documentation](https://aka.ms/AA7jfor) to learn more.' + 'The setting "python.pythonPath" defined in your workspace settings is now deprecated. Do you want us to delete it from your .code-workspace file only? [Learn more](https://aka.ms/AA7jfor).' ); export const removePythonPathCodeWorkspaceAndSettingsJson = localize( 'diagnostics.removePythonPathCodeWorkspaceAndSettingsJson', - 'The setting "python.pythonPath" defined in your workspace settings is now deprecated. Do you want us to delete it from your .code-workspace file and settings.json? Refer to [this documentation](https://aka.ms/AA7jfor) to learn more.' + 'The setting "python.pythonPath" defined in your workspace settings is now deprecated. Do you want us to delete it from your .code-workspace file and settings.json? [Learn more](https://aka.ms/AA7jfor).' ); export const invalidPythonPathInDebuggerSettings = localize( 'diagnostics.invalidPythonPathInDebuggerSettings',