Skip to content

Commit a71c116

Browse files
committed
Put linter prompt behind experiment (microsoft#14731)
* Add new linter prompt * Tests * Fix typos * Show linter install message only once. * Add more tests and news
1 parent d08bd55 commit a71c116

9 files changed

Lines changed: 477 additions & 150 deletions

File tree

‎news/1 Enhancements/14760.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Put linter prompt behind an experiment flag.

‎package.nls.json‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,10 @@
100100
"Linter.enableLinter": "Enable {0}",
101101
"Linter.enablePylint": "You have a pylintrc file in your workspace. Do you want to enable pylint?",
102102
"Linter.replaceWithSelectedLinter": "Multiple linters are enabled in settings. Replace with '{0}'?",
103+
"Linter.install": "Install a linter to get error reporting.",
104+
"Linter.installPylint": "Install pylint",
105+
"Linter.installFlake8": "Install flake8",
106+
"Linter.selectLinter": "Select Linter",
103107
"Installer.noCondaOrPipInstaller": "There is no Conda or Pip installer available in the selected environment.",
104108
"Installer.noPipInstaller": "There is no Pip installer available in the selected environment.",
105109
"Installer.searchForHelp": "Search for help",

‎src/client/common/experiments/groups.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,3 +82,10 @@ export enum JoinMailingListPromptVariants {
8282
export enum SendSelectionToREPL {
8383
experiment = 'pythonSendEntireLineToREPL'
8484
}
85+
86+
// Experiment to show a prompt asking users to install or select linter
87+
export enum LinterInstallationPromptVariants {
88+
pylintFirst = 'pythonInstallPylintButtonFirst',
89+
flake8First = 'pythonInstallFlake8ButtonFirst',
90+
noPrompt = 'pythonNotDisplayLinterPrompt'
91+
}

‎src/client/common/installer/productInstaller.ts‎

Lines changed: 180 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -6,18 +6,20 @@ import { CancellationToken, OutputChannel, Uri } from 'vscode';
66
import '../../common/extensions';
77
import { IInterpreterService } from '../../interpreter/contracts';
88
import { IServiceContainer } from '../../ioc/types';
9-
import { LinterId } from '../../linters/types';
9+
import { ILinterManager, LinterId } from '../../linters/types';
1010
import { PythonEnvironment } from '../../pythonEnvironments/info';
1111
import { sendTelemetryEvent } from '../../telemetry';
1212
import { EventName } from '../../telemetry/constants';
1313
import { IApplicationShell, ICommandManager, IWorkspaceService } from '../application/types';
1414
import { Commands, STANDARD_OUTPUT_CHANNEL } from '../constants';
15-
import { traceError } from '../logger';
15+
import { LinterInstallationPromptVariants } from '../experiments/groups';
16+
import { traceError, traceInfo } from '../logger';
1617
import { IPlatformService } from '../platform/types';
1718
import { IProcessServiceFactory, IPythonExecutionFactory } from '../process/types';
1819
import { ITerminalServiceFactory } from '../terminal/types';
1920
import {
2021
IConfigurationService,
22+
IExperimentsManager,
2123
IInstaller,
2224
InstallerResponse,
2325
IOutputChannel,
@@ -26,7 +28,7 @@ import {
2628
Product,
2729
ProductType
2830
} from '../types';
29-
import { Installer } from '../utils/localize';
31+
import { Common, Installer, Linters } from '../utils/localize';
3032
import { isResource, noop } from '../utils/misc';
3133
import { ProductNames } from './productNames';
3234
import {
@@ -39,14 +41,14 @@ import {
3941

4042
export { Product } from '../types';
4143

42-
export const CTagsInsllationScript =
44+
export const CTagsInstallationScript =
4345
os.platform() === 'darwin' ? 'brew install ctags' : 'sudo apt-get install exuberant-ctags';
4446

4547
export abstract class BaseInstaller {
4648
private static readonly PromptPromises = new Map<string, Promise<InstallerResponse>>();
4749
protected readonly appShell: IApplicationShell;
4850
protected readonly configService: IConfigurationService;
49-
private readonly workspaceService: IWorkspaceService;
51+
protected readonly workspaceService: IWorkspaceService;
5052
private readonly productService: IProductService;
5153

5254
constructor(protected serviceContainer: IServiceContainer, protected outputChannel: OutputChannel) {
@@ -168,8 +170,8 @@ export class CTagsInstaller extends BaseInstaller {
168170
.get<ITerminalServiceFactory>(ITerminalServiceFactory)
169171
.getTerminalService(resource);
170172
terminalService
171-
.sendCommand(CTagsInsllationScript, [])
172-
.catch((ex) => traceError(`Failed to install ctags. Script sent '${CTagsInsllationScript}', ${ex}`));
173+
.sendCommand(CTagsInstallationScript, [])
174+
.catch((ex) => traceError(`Failed to install ctags. Script sent '${CTagsInstallationScript}', ${ex}`));
173175
}
174176
return InstallerResponse.Ignore;
175177
}
@@ -230,24 +232,162 @@ export class FormatterInstaller extends BaseInstaller {
230232
}
231233

232234
export class LinterInstaller extends BaseInstaller {
235+
// This is a hack, really we should be handling this in a service that
236+
// controls the prompts we show. The issue here was that if we show
237+
// a prompt to install pylint and flake8, and user selects flake8
238+
// we immediately show this prompt again saying install flake8, while the
239+
// installation is on going.
240+
private static promptSeen: boolean = false;
241+
private readonly experimentsManager: IExperimentsManager;
242+
private readonly linterManager: ILinterManager;
243+
244+
constructor(protected serviceContainer: IServiceContainer, protected outputChannel: OutputChannel) {
245+
super(serviceContainer, outputChannel);
246+
this.experimentsManager = serviceContainer.get<IExperimentsManager>(IExperimentsManager);
247+
this.linterManager = serviceContainer.get<ILinterManager>(ILinterManager);
248+
}
249+
250+
public static reset() {
251+
// Read notes where this is defined.
252+
LinterInstaller.promptSeen = false;
253+
}
254+
233255
protected async promptToInstallImplementation(
234256
product: Product,
235257
resource?: Uri,
236258
cancel?: CancellationToken
237259
): Promise<InstallerResponse> {
260+
// This is a hack, really we should be handling this in a service that
261+
// controls the prompts we show. The issue here was that if we show
262+
// a prompt to install pylint and flake8, and user selects flake8
263+
// we immediately show this prompt again saying install flake8, while the
264+
// installation is on going.
265+
if (LinterInstaller.promptSeen) {
266+
return InstallerResponse.Ignore;
267+
}
268+
269+
LinterInstaller.promptSeen = true;
270+
271+
// Conditions to use experiment prompt:
272+
// 1. There should be no linter set in any scope
273+
// 2. The default linter should be pylint
274+
275+
if (!this.isLinterSetInAnyScope() && product === Product.pylint) {
276+
if (this.experimentsManager.inExperiment(LinterInstallationPromptVariants.noPrompt)) {
277+
// We won't show a prompt, so tell the extension to treat as though user
278+
// ignored the prompt.
279+
sendTelemetryEvent(EventName.LINTER_INSTALL_PROMPT, undefined, {
280+
prompt: 'noPrompt'
281+
});
282+
283+
const productName = ProductNames.get(product)!;
284+
traceInfo(`Linter ${productName} is not installed.`);
285+
286+
return InstallerResponse.Ignore;
287+
} else if (this.experimentsManager.inExperiment(LinterInstallationPromptVariants.pylintFirst)) {
288+
return this.newPromptForInstallation(true, resource, cancel);
289+
} else if (this.experimentsManager.inExperiment(LinterInstallationPromptVariants.flake8First)) {
290+
return this.newPromptForInstallation(false, resource, cancel);
291+
}
292+
}
293+
294+
sendTelemetryEvent(EventName.LINTER_INSTALL_PROMPT, undefined, {
295+
prompt: 'old'
296+
});
297+
return this.oldPromptForInstallation(product, resource, cancel);
298+
}
299+
300+
/**
301+
* For installers that want to avoid prompting the user over and over, they can make use of a
302+
* persisted true/false value representing user responses to 'stop showing this prompt'. This method
303+
* gets the persisted value given the installer-defined key.
304+
*
305+
* @param key Key to use to get a persisted response value, each installer must define this for themselves.
306+
* @returns Boolean: The current state of the stored response key given.
307+
*/
308+
protected getStoredResponse(key: string): boolean {
309+
const factory = this.serviceContainer.get<IPersistentStateFactory>(IPersistentStateFactory);
310+
const state = factory.createGlobalPersistentState<boolean | undefined>(key, undefined);
311+
return state.value === true;
312+
}
313+
314+
private async newPromptForInstallation(pylintFirst: boolean, resource?: Uri, cancel?: CancellationToken) {
315+
const productName = ProductNames.get(Product.pylint)!;
316+
317+
// User has already set to ignore this prompt
318+
const disableLinterInstallPromptKey = `${productName}_DisableLinterInstallPrompt`;
319+
if (this.getStoredResponse(disableLinterInstallPromptKey) === true) {
320+
return InstallerResponse.Ignore;
321+
}
322+
323+
// Check if the linter settings has Pylint or flake8 pointing to executables.
324+
// If the settings point to executables then we can't install. Defer to old Prompt.
325+
if (
326+
!this.isExecutableAModule(Product.pylint, resource) ||
327+
!this.isExecutableAModule(Product.flake8, resource)
328+
) {
329+
return this.oldPromptForInstallation(Product.pylint, resource, cancel);
330+
}
331+
332+
const installPylint = Linters.installPylint();
333+
const installFlake8 = Linters.installFlake8();
334+
const doNotShowAgain = Common.doNotShowAgain();
335+
336+
const options = pylintFirst
337+
? [installPylint, installFlake8, doNotShowAgain]
338+
: [installFlake8, installPylint, doNotShowAgain];
339+
const message = Linters.installMessage();
340+
const prompt = pylintFirst ? 'pylintFirst' : 'flake8first';
341+
342+
sendTelemetryEvent(EventName.LINTER_INSTALL_PROMPT, undefined, {
343+
prompt
344+
});
345+
346+
const response = await this.appShell.showInformationMessage(message, ...options);
347+
348+
if (response === installPylint) {
349+
sendTelemetryEvent(EventName.LINTER_INSTALL_PROMPT_ACTION, undefined, {
350+
prompt,
351+
action: 'installPylint'
352+
});
353+
return this.install(Product.pylint, resource, cancel);
354+
} else if (response === installFlake8) {
355+
sendTelemetryEvent(EventName.LINTER_INSTALL_PROMPT_ACTION, undefined, {
356+
prompt,
357+
action: 'installFlake8'
358+
});
359+
await this.linterManager.setActiveLintersAsync([Product.flake8], resource);
360+
return this.install(Product.flake8, resource, cancel);
361+
} else if (response === doNotShowAgain) {
362+
sendTelemetryEvent(EventName.LINTER_INSTALL_PROMPT_ACTION, undefined, {
363+
prompt,
364+
action: 'disablePrompt'
365+
});
366+
await this.setStoredResponse(disableLinterInstallPromptKey, true);
367+
return InstallerResponse.Ignore;
368+
}
369+
370+
sendTelemetryEvent(EventName.LINTER_INSTALL_PROMPT_ACTION, undefined, {
371+
prompt,
372+
action: 'close'
373+
});
374+
return InstallerResponse.Ignore;
375+
}
376+
377+
private async oldPromptForInstallation(product: Product, resource?: Uri, cancel?: CancellationToken) {
238378
const isPylint = product === Product.pylint;
239379

240380
const productName = ProductNames.get(product)!;
241-
const install = 'Install';
242-
const disableInstallPrompt = 'Do not show again';
381+
const install = Common.install();
382+
const doNotShowAgain = Common.doNotShowAgain();
243383
const disableLinterInstallPromptKey = `${productName}_DisableLinterInstallPrompt`;
244-
const selectLinter = 'Select Linter';
384+
const selectLinter = Linters.selectLinter();
245385

246386
if (isPylint && this.getStoredResponse(disableLinterInstallPromptKey) === true) {
247387
return InstallerResponse.Ignore;
248388
}
249389

250-
const options = isPylint ? [selectLinter, disableInstallPrompt] : [selectLinter];
390+
const options = isPylint ? [selectLinter, doNotShowAgain] : [selectLinter];
251391

252392
let message = `Linter ${productName} is not installed.`;
253393
if (this.isExecutableAModule(product, resource)) {
@@ -263,7 +403,7 @@ export class LinterInstaller extends BaseInstaller {
263403
action: 'install'
264404
});
265405
return this.install(product, resource, cancel);
266-
} else if (response === disableInstallPrompt) {
406+
} else if (response === doNotShowAgain) {
267407
await this.setStoredResponse(disableLinterInstallPromptKey, true);
268408
sendTelemetryEvent(EventName.LINTER_NOT_INSTALLED_PROMPT, undefined, {
269409
tool: productName as LinterId,
@@ -280,18 +420,34 @@ export class LinterInstaller extends BaseInstaller {
280420
return InstallerResponse.Ignore;
281421
}
282422

283-
/**
284-
* For installers that want to avoid prompting the user over and over, they can make use of a
285-
* persisted true/false value representing user responses to 'stop showing this prompt'. This method
286-
* gets the persisted value given the installer-defined key.
287-
*
288-
* @param key Key to use to get a persisted response value, each installer must define this for themselves.
289-
* @returns Boolean: The current state of the stored response key given.
290-
*/
291-
protected getStoredResponse(key: string): boolean {
292-
const factory = this.serviceContainer.get<IPersistentStateFactory>(IPersistentStateFactory);
293-
const state = factory.createGlobalPersistentState<boolean | undefined>(key, undefined);
294-
return state.value === true;
423+
private isLinterSetInAnyScope() {
424+
const config = this.workspaceService.getConfiguration('python');
425+
if (config) {
426+
const keys = [
427+
'linting.pylintEnabled',
428+
'linting.flake8Enabled',
429+
'linting.banditEnabled',
430+
'linting.mypyEnabled',
431+
'linting.pycodestyleEnabled',
432+
'linting.prospectorEnabled',
433+
'linting.pydocstyleEnabled',
434+
'linting.pylamaEnabled'
435+
];
436+
437+
const values = keys.map((key) => {
438+
const value = config.inspect<boolean>(key);
439+
if (value) {
440+
if (value.globalValue || value.workspaceValue || value.workspaceFolderValue) {
441+
return 'linter set';
442+
}
443+
}
444+
return 'no info';
445+
});
446+
447+
return values.includes('linter set');
448+
}
449+
450+
return false;
295451
}
296452

297453
/**

‎src/client/common/utils/localize.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -284,6 +284,11 @@ export namespace Linters {
284284
'Linter.replaceWithSelectedLinter',
285285
"Multiple linters are enabled in settings. Replace with '{0}'?"
286286
);
287+
288+
export const installMessage = localize('Linter.install', 'Install a linter to get error reporting.');
289+
export const installPylint = localize('Linter.installPylint', 'Install pylint');
290+
export const installFlake8 = localize('Linter.installFlake8', 'Install flake8');
291+
export const selectLinter = localize('Linter.selectLinter', 'Select Linter');
287292
}
288293

289294
export namespace Installer {

‎src/client/telemetry/constants.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,8 @@ export enum EventName {
111111
SELECT_LINTER = 'LINTING.SELECT',
112112

113113
LINTER_NOT_INSTALLED_PROMPT = 'LINTER_NOT_INSTALLED_PROMPT',
114+
LINTER_INSTALL_PROMPT = 'LINTER_INSTALL_PROMPT',
115+
LINTER_INSTALL_PROMPT_ACTION = 'LINTER_INSTALL_PROMPT_ACTION',
114116
CONFIGURE_AVAILABLE_LINTER_PROMPT = 'CONFIGURE_AVAILABLE_LINTER_PROMPT',
115117
HASHED_PACKAGE_NAME = 'HASHED_PACKAGE_NAME',
116118
HASHED_PACKAGE_PERF = 'HASHED_PACKAGE_PERF',

‎src/client/telemetry/index.ts‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -779,6 +779,39 @@ export interface IEventNamePropertyMapping {
779779
*/
780780
action: 'select' | 'disablePrompt' | 'install';
781781
};
782+
783+
/**
784+
* Telemetry event sent before showing the linter prompt to install
785+
* pylint or flake8.
786+
*/
787+
[EventName.LINTER_INSTALL_PROMPT]: {
788+
/**
789+
* Identify which prompt was shown.
790+
*
791+
* @type {('old' | 'noPrompt' | 'pylintFirst' | 'flake8first')}
792+
*/
793+
prompt: 'old' | 'noPrompt' | 'pylintFirst' | 'flake8first';
794+
};
795+
796+
/**
797+
* Telemetry event sent after user had selected one of the options
798+
* provided by the linter prompt.
799+
*/
800+
[EventName.LINTER_INSTALL_PROMPT_ACTION]: {
801+
/**
802+
* Identify which prompt was shown.
803+
*
804+
* @type {('pylintFirst' | 'flake8first')}
805+
*/
806+
prompt: 'pylintFirst' | 'flake8first';
807+
808+
/**
809+
* Which of the following actions did user select
810+
*
811+
* @type {('pylint' | 'flake8' | 'disablePrompt' | 'close')}
812+
*/
813+
action: 'installPylint' | 'installFlake8' | 'disablePrompt' | 'close';
814+
};
782815
/**
783816
* Telemetry event sent when installing modules
784817
*/

0 commit comments

Comments
 (0)