diff --git a/.vscode/launch.json b/.vscode/launch.json index 11b9ecc5c193..d8c2042447d7 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -166,4 +166,4 @@ ] } ] -} +} \ No newline at end of file diff --git a/news/1 Enhancements/3349.md b/news/1 Enhancements/3349.md new file mode 100644 index 000000000000..4c29e1dc881f --- /dev/null +++ b/news/1 Enhancements/3349.md @@ -0,0 +1 @@ +Allow users to request the 'Install missing Linter' prompt to not show again for pylint. \ No newline at end of file diff --git a/src/client/common/installer/productInstaller.ts b/src/client/common/installer/productInstaller.ts index b0256848ea22..902b789fb7e7 100644 --- a/src/client/common/installer/productInstaller.ts +++ b/src/client/common/installer/productInstaller.ts @@ -11,7 +11,10 @@ import { STANDARD_OUTPUT_CHANNEL } from '../constants'; import { IPlatformService } from '../platform/types'; import { IProcessServiceFactory, IPythonExecutionFactory } from '../process/types'; import { ITerminalServiceFactory } from '../terminal/types'; -import { IConfigurationService, IInstaller, ILogger, InstallerResponse, IOutputChannel, ModuleNamePurpose, Product, ProductType } from '../types'; +import { + IConfigurationService, IInstaller, ILogger, InstallerResponse, IOutputChannel, + IPersistentStateFactory, ModuleNamePurpose, Product, ProductType +} from '../types'; import { ProductNames } from './productNames'; import { IInstallationChannelManager, IProductPathService, IProductService } from './types'; @@ -88,6 +91,7 @@ export abstract class BaseInstaller { .catch(() => false); } } + protected abstract promptToInstallImplementation(product: Product, resource?: Uri): Promise; protected getExecutableNameFromSettings(product: Product, resource?: Uri): string { const productType = this.productService.getProductType(product); @@ -168,12 +172,21 @@ export class FormatterInstaller extends BaseInstaller { export class LinterInstaller extends BaseInstaller { protected async promptToInstallImplementation(product: Product, resource?: Uri): Promise { + const isPylint = product === Product.pylint; + const productName = ProductNames.get(product)!; const install = 'Install'; const disableAllLinting = 'Disable linting'; const disableThisLinter = `Disable ${productName}`; + const disableInstallPrompt = 'Do not show again'; + const disableLinterInstallPromptKey = `${productName}_DisableLinterInstallPrompt`; + + if (isPylint && this.getStoredResponse(disableLinterInstallPromptKey) === true) { + return InstallerResponse.Ignore; + } + + const options = isPylint ? [disableThisLinter, disableAllLinting, disableInstallPrompt] : [disableThisLinter, disableAllLinting]; - const options = [disableThisLinter, disableAllLinting]; let message = `Linter ${productName} is not installed.`; if (this.isExecutableAModule(product, resource)) { options.splice(0, 0, install); @@ -185,7 +198,11 @@ export class LinterInstaller extends BaseInstaller { const response = await this.appShell.showErrorMessage(message, ...options); if (response === install) { return this.install(product, resource); + } else if (response === disableInstallPrompt) { + await this.setStoredResponse(disableLinterInstallPromptKey, true); + return InstallerResponse.Ignore; } + const lm = this.serviceContainer.get(ILinterManager); if (response === disableAllLinting) { await lm.enableLintingAsync(false); @@ -196,6 +213,37 @@ export class LinterInstaller extends BaseInstaller { } return InstallerResponse.Ignore; } + + /** + * For installers that want to avoid prompting the user over and over, they can make use of a + * persisted true/false value representing user responses to 'stop showing this prompt'. This method + * gets the persisted value given the installer-defined key. + * + * @param key Key to use to get a persisted response value, each installer must define this for themselves. + * @returns Boolean: The current state of the stored response key given. + */ + private getStoredResponse(key: string): boolean { + const factory = this.serviceContainer.get(IPersistentStateFactory); + const state = factory.createGlobalPersistentState(key, undefined); + return state.value; + } + + /** + * For installers that want to avoid prompting the user over and over, they can make use of a + * persisted true/false value representing user responses to 'stop showing this prompt'. This + * method will set that persisted value given the installer-defined key. + * + * @param key Key to use to get a persisted response value, each installer must define this for themselves. + * @param value Boolean value to store for the user - if they choose to not be prompted again for instance. + * @returns Boolean: The current state of the stored response key given. + */ + private async setStoredResponse(key: string, value: boolean): Promise { + const factory = this.serviceContainer.get(IPersistentStateFactory); + const state = factory.createGlobalPersistentState(key, undefined); + if (state && state.value !== value) { + await state.updateValue(value); + } + } } export class TestFrameworkInstaller extends BaseInstaller { diff --git a/src/test/common/installer/installer.invalidPath.unit.test.ts b/src/test/common/installer/installer.invalidPath.unit.test.ts index 7b26a3253d67..65fc3cb40765 100644 --- a/src/test/common/installer/installer.invalidPath.unit.test.ts +++ b/src/test/common/installer/installer.invalidPath.unit.test.ts @@ -13,7 +13,7 @@ import '../../../client/common/extensions'; import { ProductInstaller } from '../../../client/common/installer/productInstaller'; import { ProductService } from '../../../client/common/installer/productService'; import { IProductPathService, IProductService } from '../../../client/common/installer/types'; -import { Product } from '../../../client/common/types'; +import { IPersistentState, IPersistentStateFactory, Product } from '../../../client/common/types'; import { getNamesAndValues } from '../../../client/common/utils/enum'; import { IServiceContainer } from '../../../client/ioc/types'; @@ -30,6 +30,8 @@ suite('Module Installer - Invalid Paths', () => { let app: TypeMoq.IMock; let workspaceService: TypeMoq.IMock; let productPathService: TypeMoq.IMock; + let persistentState: TypeMoq.IMock; + setup(() => { serviceContainer = TypeMoq.Mock.ofType(); const outputChannel = TypeMoq.Mock.ofType(); @@ -43,6 +45,9 @@ suite('Module Installer - Invalid Paths', () => { productPathService = TypeMoq.Mock.ofType(); serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IProductPathService), TypeMoq.It.isAny())).returns(() => productPathService.object); + persistentState = TypeMoq.Mock.ofType(); + serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IPersistentStateFactory), TypeMoq.It.isAny())).returns(() => persistentState.object); + installer = new ProductInstaller(serviceContainer.object, outputChannel.object); }); @@ -74,7 +79,12 @@ suite('Module Installer - Invalid Paths', () => { }) .returns(() => Promise.resolve(undefined)) .verifiable(TypeMoq.Times.exactly(1)); - + const persistValue = TypeMoq.Mock.ofType>(); + persistValue.setup(pv => pv.value).returns(() => false); + persistValue.setup(pv => pv.updateValue(TypeMoq.It.isValue(true))); + persistentState.setup(ps => + ps.createGlobalPersistentState(TypeMoq.It.isAnyString(), TypeMoq.It.isValue(undefined)) + ).returns(() => persistValue.object); await installer.promptToInstall(product.value, resource); productPathService.verifyAll(); }); diff --git a/src/test/common/installer/installer.unit.test.ts b/src/test/common/installer/installer.unit.test.ts index 2187bef2da78..9176c0d4a713 100644 --- a/src/test/common/installer/installer.unit.test.ts +++ b/src/test/common/installer/installer.unit.test.ts @@ -11,15 +11,20 @@ import { IApplicationShell, IWorkspaceService } from '../../../client/common/app import '../../../client/common/extensions'; import { ProductInstaller } from '../../../client/common/installer/productInstaller'; import { ProductService } from '../../../client/common/installer/productService'; -import { IInstallationChannelManager, IModuleInstaller, IProductPathService, IProductService } from '../../../client/common/installer/types'; -import { IDisposableRegistry, ILogger, InstallerResponse, ModuleNamePurpose, Product } from '../../../client/common/types'; +import { + IInstallationChannelManager, IModuleInstaller, IProductPathService, IProductService +} from '../../../client/common/installer/types'; +import { + IDisposableRegistry, ILogger, InstallerResponse, IPersistentState, + IPersistentStateFactory, ModuleNamePurpose, Product, ProductType +} from '../../../client/common/types'; import { createDeferred, Deferred } from '../../../client/common/utils/async'; import { getNamesAndValues } from '../../../client/common/utils/enum'; import { IServiceContainer } from '../../../client/ioc/types'; use(chaiAsPromised); -suite('Module Installer', () => { +suite('Module Installer only', () => { [undefined, Uri.file('resource')].forEach(resource => { getNamesAndValues(Product).forEach(product => { let disposables: Disposable[] = []; @@ -30,6 +35,9 @@ suite('Module Installer', () => { let app: TypeMoq.IMock; let promptDeferred: Deferred; let workspaceService: TypeMoq.IMock; + let persistentStore: TypeMoq.IMock; + const productService = new ProductService(); + setup(() => { promptDeferred = createDeferred(); serviceContainer = TypeMoq.Mock.ofType(); @@ -37,13 +45,15 @@ suite('Module Installer', () => { disposables = []; serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IDisposableRegistry), TypeMoq.It.isAny())).returns(() => disposables); - serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IProductService), TypeMoq.It.isAny())).returns(() => new ProductService()); + serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IProductService), TypeMoq.It.isAny())).returns(() => productService); installationChannel = TypeMoq.Mock.ofType(); serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IInstallationChannelManager), TypeMoq.It.isAny())).returns(() => installationChannel.object); app = TypeMoq.Mock.ofType(); serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IApplicationShell), TypeMoq.It.isAny())).returns(() => app.object); workspaceService = TypeMoq.Mock.ofType(); serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IWorkspaceService), TypeMoq.It.isAny())).returns(() => workspaceService.object); + persistentStore = TypeMoq.Mock.ofType(); + serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IPersistentStateFactory), TypeMoq.It.isAny())).returns(() => persistentStore.object); moduleInstaller = TypeMoq.Mock.ofType(); // tslint:disable-next-line:no-any @@ -113,13 +123,22 @@ suite('Module Installer', () => { } }); if (product.value !== Product.unittest) { - test(`Ensure the prompt is displayed only once, untill the prompt is closed, ${product.name} (${resource ? 'With a resource' : 'without a resource'})`, async () => { + test(`Ensure the prompt is displayed only once, until the prompt is closed, ${product.name} (${resource ? 'With a resource' : 'without a resource'})`, async () => { workspaceService.setup(w => w.getWorkspaceFolder(TypeMoq.It.isValue(resource!))) .returns(() => TypeMoq.Mock.ofType().object) .verifiable(TypeMoq.Times.exactly(resource ? 5 : 0)); app.setup(a => a.showErrorMessage(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())) - .returns(() => promptDeferred.promise) + .returns( + () => { + return promptDeferred.promise; + }) .verifiable(TypeMoq.Times.once()); + const persistVal = TypeMoq.Mock.ofType>(); + persistVal.setup(p => p.value).returns(() => false); + persistVal.setup(p => p.updateValue(TypeMoq.It.isValue(true))); + persistentStore.setup(ps => + ps.createGlobalPersistentState(TypeMoq.It.isAnyString(), TypeMoq.It.isValue(undefined)) + ).returns(() => persistVal.object); // Display first prompt. installer.promptToInstall(product.value, resource).ignoreErrors(); @@ -133,6 +152,103 @@ suite('Module Installer', () => { app.verifyAll(); workspaceService.verifyAll(); }); + if (product.value === Product.pylint) { + test(`Ensure the install prompt is not displayed when the user requests it not be shown again, ${product.name} (${resource ? 'With a resource' : 'without a resource'})`, async () => { + workspaceService.setup(w => w.getWorkspaceFolder(TypeMoq.It.isValue(resource!))) + .returns(() => TypeMoq.Mock.ofType().object) + .verifiable(TypeMoq.Times.exactly(resource ? 2 : 0)); + app.setup(a => + a.showErrorMessage( + TypeMoq.It.isAnyString(), + TypeMoq.It.isValue('Install'), + TypeMoq.It.isValue(`Disable ${product.name}`), + TypeMoq.It.isValue('Disable linting'), + TypeMoq.It.isValue('Do not show again'))) + .returns( + async () => { + return 'Do not show again'; + }) + .verifiable(TypeMoq.Times.once()); + const persistVal = TypeMoq.Mock.ofType>(); + let mockPersistVal = false; + persistVal.setup(p => p.value).returns(() => { + return mockPersistVal; + }); + persistVal.setup(p => p.updateValue(TypeMoq.It.isValue(true))) + .returns(() => { + mockPersistVal = true; + return Promise.resolve(); + }).verifiable(TypeMoq.Times.once()); + persistentStore.setup(ps => + ps.createGlobalPersistentState(TypeMoq.It.isAnyString(), TypeMoq.It.isValue(undefined)) + ).returns(() => { + return persistVal.object; + }).verifiable(TypeMoq.Times.exactly(3)); + + // Display first prompt. + const initialResponse = await installer.promptToInstall(product.value, resource); + + // Display a second prompt. + const secondResponse = await installer.promptToInstall(product.value, resource); + + expect(initialResponse).to.be.equal(InstallerResponse.Ignore); + expect(secondResponse).to.be.equal(InstallerResponse.Ignore); + + app.verifyAll(); + workspaceService.verifyAll(); + persistentStore.verifyAll(); + persistVal.verifyAll(); + }); + } else if (productService.getProductType(product.value) === ProductType.Linter) { + test(`Ensure the 'do not show again' prompt isn't shown for non-pylint linters, ${product.name} (${resource ? 'With a resource' : 'without a resource'})`, async () => { + workspaceService.setup(w => w.getWorkspaceFolder(TypeMoq.It.isValue(resource!))) + .returns(() => TypeMoq.Mock.ofType().object); + app.setup(a => + a.showErrorMessage( + TypeMoq.It.isAnyString(), + TypeMoq.It.isValue('Install'), + TypeMoq.It.isValue(`Disable ${product.name}`), + TypeMoq.It.isValue('Disable linting'))) + .returns( + async () => { + return undefined; + }) + .verifiable(TypeMoq.Times.once()); + app.setup(a => + a.showErrorMessage( + TypeMoq.It.isAnyString(), + TypeMoq.It.isValue('Install'), + TypeMoq.It.isValue(`Disable ${product.name}`), + TypeMoq.It.isValue('Disable linting'), + TypeMoq.It.isValue('Do not show again'))) + .returns( + async () => { + return undefined; + }) + .verifiable(TypeMoq.Times.never()); + const persistVal = TypeMoq.Mock.ofType>(); + let mockPersistVal = false; + persistVal.setup(p => p.value).returns(() => { + return mockPersistVal; + }); + persistVal.setup(p => p.updateValue(TypeMoq.It.isValue(true))) + .returns(() => { + mockPersistVal = true; + return Promise.resolve(); + }); + persistentStore.setup(ps => + ps.createGlobalPersistentState(TypeMoq.It.isAnyString(), TypeMoq.It.isValue(undefined)) + ).returns(() => { + return persistVal.object; + }); + + // Display the prompt. + await installer.promptToInstall(product.value, resource); + + // we're just ensuring the 'disable pylint' prompt never appears... + app.verifyAll(); + }); + } test(`Ensure the prompt is displayed again when previous prompt has been closed, ${product.name} (${resource ? 'With a resource' : 'without a resource'})`, async () => { workspaceService.setup(w => w.getWorkspaceFolder(TypeMoq.It.isValue(resource!))) .returns(() => TypeMoq.Mock.ofType().object) @@ -140,6 +256,12 @@ suite('Module Installer', () => { app.setup(a => a.showErrorMessage(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())) .returns(() => Promise.resolve(undefined)) .verifiable(TypeMoq.Times.exactly(3)); + const persistVal = TypeMoq.Mock.ofType>(); + persistVal.setup(p => p.value).returns(() => false); + persistVal.setup(p => p.updateValue(TypeMoq.It.isValue(true))); + persistentStore.setup(ps => + ps.createGlobalPersistentState(TypeMoq.It.isAnyString(), TypeMoq.It.isValue(undefined)) + ).returns(() => persistVal.object); await installer.promptToInstall(product.value, resource); await installer.promptToInstall(product.value, resource);