Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ensure all diagnostic services do not handle the same messages more than once #4038

Merged
merged 7 commits into from
Jan 18, 2019
Merged

Ensure all diagnostic services do not handle the same messages more than once #4038

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
abe1ed4
Fix issues
DonJayamanne Jan 17, 2019
1e08d6a
Ensure all diagnostic checks accept resource args
DonJayamanne Jan 17, 2019
606f57e
Clear cache when testing
DonJayamanne Jan 17, 2019
eacb4a2
LInter issues
DonJayamanne Jan 17, 2019
6532d05
Address code review comments
DonJayamanne Jan 18, 2019
13680ec
Address code review
DonJayamanne Jan 18, 2019
ec69daa
Fixed tests
DonJayamanne Jan 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 38 additions & 3 deletions src/client/application/diagnostics/base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import { injectable, unmanaged } from 'inversify';
import { DiagnosticSeverity } from 'vscode';
import { IWorkspaceService } from '../../common/application/types';
import { Resource } from '../../common/types';
import { IServiceContainer } from '../../ioc/types';
import { sendTelemetryEvent } from '../../telemetry';
Expand All @@ -21,15 +22,49 @@ export abstract class BaseDiagnostic implements IDiagnostic {

@injectable()
export abstract class BaseDiagnosticsService implements IDiagnosticsService {
protected static handledDiagnosticCodeKeys: string[] = [];
protected readonly filterService: IDiagnosticFilterService;
constructor(@unmanaged() private readonly supportedDiagnosticCodes: string[],
@unmanaged() protected serviceContainer: IServiceContainer) {
constructor(
@unmanaged() private readonly supportedDiagnosticCodes: string[],
@unmanaged() protected serviceContainer: IServiceContainer
) {
this.filterService = serviceContainer.get<IDiagnosticFilterService>(IDiagnosticFilterService);
}
public abstract diagnose(resource: Resource): Promise<IDiagnostic[]>;
public abstract handle(diagnostics: IDiagnostic[]): Promise<void>;
public async handle(diagnostics: IDiagnostic[]): Promise<void> {
if (diagnostics.length === 0) {
return;
}
const diagnosticsToHandle = diagnostics.filter(item => {
const key = this.getDiagnosticsKey(item);
if (BaseDiagnosticsService.handledDiagnosticCodeKeys.indexOf(key) !== -1) {
return false;
}
BaseDiagnosticsService.handledDiagnosticCodeKeys.push(key);
return true;
});
await this.onHandle(diagnosticsToHandle);
}
public async canHandle(diagnostic: IDiagnostic): Promise<boolean> {
sendTelemetryEvent(EventName.DIAGNOSTICS_MESSAGE, undefined, { code: diagnostic.code });
return this.supportedDiagnosticCodes.filter(item => item === diagnostic.code).length > 0;
}
protected abstract onHandle(diagnostics: IDiagnostic[]): Promise<void>;
/**
* Returns a key used to keep track of whether a diagnostic was handled or not.
* So as to prevent handling/displaying messages multiple times for the same diagnostic.
*
* @protected
* @param {IDiagnostic} diagnostic
* @returns {string}
* @memberof BaseDiagnosticsService
*/
protected getDiagnosticsKey(diagnostic: IDiagnostic): string {
if (diagnostic.scope === DiagnosticScope.Global) {
return diagnostic.code;
}
const workspace = this.serviceContainer.get<IWorkspaceService>(IWorkspaceService);
const workspaceFolder = diagnostic.resource ? workspace.getWorkspaceFolder(diagnostic.resource) : undefined;
DonJayamanne marked this conversation as resolved.
Show resolved Hide resolved
return `${diagnostic.code}dbe75733-0407-4124-a1b2-ca769dc30523${workspaceFolder ? workspaceFolder.uri.fsPath : ''}`;
}
}
25 changes: 16 additions & 9 deletions src/client/application/diagnostics/checks/envPathVariable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,19 @@ import { DiagnosticCodes } from '../constants';
import { DiagnosticCommandPromptHandlerServiceId, MessageCommandPrompt } from '../promptHandler';
import { DiagnosticScope, IDiagnostic, IDiagnosticHandlerService } from '../types';

const InvalidEnvPathVariableMessage = 'The environment variable \'{0}\' seems to have some paths containing the \'"\' character.' +
const InvalidEnvPathVariableMessage =
'The environment variable \'{0}\' seems to have some paths containing the \'"\' character.' +
' The existence of such a character is known to have caused the {1} extension to not load. If the extension fails to load please modify your paths to remove this \'"\' character.';

export class InvalidEnvironmentPathVariableDiagnostic extends BaseDiagnostic {
constructor(message: string, resource: Resource) {
super(DiagnosticCodes.InvalidEnvironmentPathVariableDiagnostic,
message, DiagnosticSeverity.Warning, DiagnosticScope.Global, resource);
super(
DiagnosticCodes.InvalidEnvironmentPathVariableDiagnostic,
message,
DiagnosticSeverity.Warning,
DiagnosticScope.Global,
resource
);
}
}

Expand All @@ -35,20 +41,21 @@ export class EnvironmentPathVariableDiagnosticsService extends BaseDiagnosticsSe
constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) {
super([DiagnosticCodes.InvalidEnvironmentPathVariableDiagnostic], serviceContainer);
this.platform = this.serviceContainer.get<IPlatformService>(IPlatformService);
this.messageService = serviceContainer.get<IDiagnosticHandlerService<MessageCommandPrompt>>(IDiagnosticHandlerService, DiagnosticCommandPromptHandlerServiceId);
this.messageService = serviceContainer.get<IDiagnosticHandlerService<MessageCommandPrompt>>(
IDiagnosticHandlerService,
DiagnosticCommandPromptHandlerServiceId
);
}
public async diagnose(resource: Resource): Promise<IDiagnostic[]> {
if (this.platform.isWindows &&
this.doesPathVariableHaveInvalidEntries()) {
if (this.platform.isWindows && this.doesPathVariableHaveInvalidEntries()) {
const env = this.serviceContainer.get<IApplicationEnvironment>(IApplicationEnvironment);
const message = InvalidEnvPathVariableMessage
.format(this.platform.pathVariableName, env.extensionName);
const message = InvalidEnvPathVariableMessage.format(this.platform.pathVariableName, env.extensionName);
return [new InvalidEnvironmentPathVariableDiagnostic(message, resource)];
} else {
return [];
}
}
public async handle(diagnostics: IDiagnostic[]): Promise<void> {
protected async onHandle(diagnostics: IDiagnostic[]): Promise<void> {
// This class can only handle one type of diagnostic, hence just use first item in list.
if (diagnostics.length === 0 || !this.canHandle(diagnostics[0])) {
return;
Expand Down
39 changes: 28 additions & 11 deletions src/client/application/diagnostics/checks/invalidDebuggerType.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,20 @@ import { DiagnosticCodes } from '../constants';
import { DiagnosticCommandPromptHandlerServiceId, MessageCommandPrompt } from '../promptHandler';
import { DiagnosticScope, IDiagnostic, IDiagnosticHandlerService } from '../types';

const InvalidDebuggerTypeMessage = 'Your launch.json file needs to be updated to change the "pythonExperimental" debug ' +
const InvalidDebuggerTypeMessage =
'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?';

export class InvalidDebuggerTypeDiagnostic extends BaseDiagnostic {
constructor(message: string, resource: Resource) {
super(DiagnosticCodes.InvalidDebuggerTypeDiagnostic,
message, DiagnosticSeverity.Error, DiagnosticScope.WorkspaceFolder,
resource);
super(
DiagnosticCodes.InvalidDebuggerTypeDiagnostic,
message,
DiagnosticSeverity.Error,
DiagnosticScope.WorkspaceFolder,
resource
);
}
}

Expand All @@ -39,7 +44,10 @@ export class InvalidDebuggerTypeDiagnosticsService extends BaseDiagnosticsServic
protected readonly fs: IFileSystem;
constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) {
super([DiagnosticCodes.InvalidEnvironmentPathVariableDiagnostic], serviceContainer);
this.messageService = serviceContainer.get<IDiagnosticHandlerService<MessageCommandPrompt>>(IDiagnosticHandlerService, DiagnosticCommandPromptHandlerServiceId);
this.messageService = serviceContainer.get<IDiagnosticHandlerService<MessageCommandPrompt>>(
IDiagnosticHandlerService,
DiagnosticCommandPromptHandlerServiceId
);
const cmdManager = serviceContainer.get<ICommandManager>(ICommandManager);
this.fs = this.serviceContainer.get<IFileSystem>(IFileSystem);
cmdManager.registerCommand(CommandName, this.fixLaunchJson, this);
Expand All @@ -51,7 +59,7 @@ export class InvalidDebuggerTypeDiagnosticsService extends BaseDiagnosticsServic
return [];
}
}
public async handle(diagnostics: IDiagnostic[]): Promise<void> {
protected async onHandle(diagnostics: IDiagnostic[]): Promise<void> {
// This class can only handle one type of diagnostic, hence just use first item in list.
if (diagnostics.length === 0 || !this.canHandle(diagnostics[0])) {
return;
Expand All @@ -61,7 +69,10 @@ export class InvalidDebuggerTypeDiagnosticsService extends BaseDiagnosticsServic
const options = [
{
prompt: 'Yes, update launch.json',
command: commandFactory.createCommand(diagnostic, { type: 'executeVSCCommand', options: 'python.debugger.replaceExperimental' })
command: commandFactory.createCommand(diagnostic, {
type: 'executeVSCCommand',
options: 'python.debugger.replaceExperimental'
})
},
{
prompt: 'No, I will do it later'
Expand All @@ -76,15 +87,19 @@ export class InvalidDebuggerTypeDiagnosticsService extends BaseDiagnosticsServic
return false;
}

const results = await Promise.all(workspaceService.workspaceFolders!.map(workspaceFolder => this.isExperimentalDebuggerUsedInWorkspace(workspaceFolder)));
const results = await Promise.all(
workspaceService.workspaceFolders!.map(workspaceFolder =>
this.isExperimentalDebuggerUsedInWorkspace(workspaceFolder)
)
);
return results.filter(used => used === true).length > 0;
}
private getLaunchJsonFile(workspaceFolder: WorkspaceFolder) {
return path.join(workspaceFolder.uri.fsPath, '.vscode', 'launch.json');
}
private async isExperimentalDebuggerUsedInWorkspace(workspaceFolder: WorkspaceFolder) {
const launchJson = this.getLaunchJsonFile(workspaceFolder);
if (!await this.fs.fileExists(launchJson)) {
if (!(await this.fs.fileExists(launchJson))) {
return false;
}

Expand All @@ -97,10 +112,12 @@ export class InvalidDebuggerTypeDiagnosticsService extends BaseDiagnosticsServic
return false;
}

await Promise.all(workspaceService.workspaceFolders!.map(workspaceFolder => this.fixLaunchJsonInWorkspace(workspaceFolder)));
await Promise.all(
workspaceService.workspaceFolders!.map(workspaceFolder => this.fixLaunchJsonInWorkspace(workspaceFolder))
);
}
private async fixLaunchJsonInWorkspace(workspaceFolder: WorkspaceFolder) {
if (!await this.isExperimentalDebuggerUsedInWorkspace(workspaceFolder)) {
if (!(await this.isExperimentalDebuggerUsedInWorkspace(workspaceFolder))) {
return;
}

Expand Down
99 changes: 66 additions & 33 deletions src/client/application/diagnostics/checks/invalidPythonPathInDebugger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,49 +19,64 @@ import { BaseDiagnostic, BaseDiagnosticsService } from '../base';
import { IDiagnosticsCommandFactory } from '../commands/types';
import { DiagnosticCodes } from '../constants';
import { DiagnosticCommandPromptHandlerServiceId, MessageCommandPrompt } from '../promptHandler';
import { DiagnosticScope, IDiagnostic, IDiagnosticCommand, IDiagnosticHandlerService, IInvalidPythonPathInDebuggerService } from '../types';
import {
DiagnosticScope,
IDiagnostic,
IDiagnosticCommand,
IDiagnosticHandlerService,
IInvalidPythonPathInDebuggerService
} from '../types';

export class InvalidPythonPathInDebuggerSettingsDiagnostic extends BaseDiagnostic {
constructor(resource: Resource) {
super(DiagnosticCodes.InvalidPythonPathInDebuggerSettingsDiagnostic,
Diagnostics.invalidPythonPathInDebuggerSettings(), DiagnosticSeverity.Error, DiagnosticScope.WorkspaceFolder,
resource);
super(
DiagnosticCodes.InvalidPythonPathInDebuggerSettingsDiagnostic,
Diagnostics.invalidPythonPathInDebuggerSettings(),
DiagnosticSeverity.Error,
DiagnosticScope.WorkspaceFolder,
resource
);
}
}

export class InvalidPythonPathInDebuggerLaunchDiagnostic extends BaseDiagnostic {
constructor(resource: Resource) {
super(DiagnosticCodes.InvalidPythonPathInDebuggerLaunchDiagnostic,
Diagnostics.invalidPythonPathInDebuggerLaunch(), DiagnosticSeverity.Error, DiagnosticScope.WorkspaceFolder,
resource);
super(
DiagnosticCodes.InvalidPythonPathInDebuggerLaunchDiagnostic,
Diagnostics.invalidPythonPathInDebuggerLaunch(),
DiagnosticSeverity.Error,
DiagnosticScope.WorkspaceFolder,
resource
);
}
}

export const InvalidPythonPathInDebuggerServiceId = 'InvalidPythonPathInDebuggerServiceId';

@injectable()
export class InvalidPythonPathInDebuggerService extends BaseDiagnosticsService implements IInvalidPythonPathInDebuggerService {
constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer,
export class InvalidPythonPathInDebuggerService extends BaseDiagnosticsService
implements IInvalidPythonPathInDebuggerService {
constructor(
@inject(IServiceContainer) serviceContainer: IServiceContainer,
@inject(IWorkspaceService) private readonly workspace: IWorkspaceService,
@inject(IDiagnosticsCommandFactory) private readonly commandFactory: IDiagnosticsCommandFactory,
@inject(IInterpreterHelper) private readonly interpreterHelper: IInterpreterHelper,
@inject(IConfigurationService) private readonly configService: IConfigurationService,
@inject(IDiagnosticHandlerService) @named(DiagnosticCommandPromptHandlerServiceId) protected readonly messageService: IDiagnosticHandlerService<MessageCommandPrompt>) {
super([DiagnosticCodes.InvalidPythonPathInDebuggerSettingsDiagnostic, DiagnosticCodes.InvalidPythonPathInDebuggerLaunchDiagnostic], serviceContainer);
@inject(IDiagnosticHandlerService)
@named(DiagnosticCommandPromptHandlerServiceId)
protected readonly messageService: IDiagnosticHandlerService<MessageCommandPrompt>
) {
super(
[
DiagnosticCodes.InvalidPythonPathInDebuggerSettingsDiagnostic,
DiagnosticCodes.InvalidPythonPathInDebuggerLaunchDiagnostic
],
serviceContainer
);
}
public async diagnose(_resource: Resource): Promise<IDiagnostic[]> {
return [];
}
public async handle(diagnostics: IDiagnostic[]): Promise<void> {
// This class can only handle one type of diagnostic, hence just use first item in list.
if (diagnostics.length === 0 || !this.canHandle(diagnostics[0])) {
return;
}
const diagnostic = diagnostics[0];
const commandPrompts = this.getCommandPrompts(diagnostic);

await this.messageService.handle(diagnostic, { commandPrompts });
}
public async validatePythonPath(pythonPath?: string, resource?: Uri) {
pythonPath = pythonPath ? this.resolveVariables(pythonPath, resource) : undefined;
let pathInLaunchJson = true;
Expand All @@ -85,6 +100,16 @@ export class InvalidPythonPathInDebuggerService extends BaseDiagnosticsService i
}
return false;
}
protected async onHandle(diagnostics: IDiagnostic[]): Promise<void> {
// This class can only handle one type of diagnostic, hence just use first item in list.
if (diagnostics.length === 0 || !this.canHandle(diagnostics[0])) {
return;
}
const diagnostic = diagnostics[0];
const commandPrompts = this.getCommandPrompts(diagnostic);

await this.messageService.handle(diagnostic, { commandPrompts });
}
protected resolveVariables(pythonPath: string, resource: Uri | undefined): string {
const workspaceFolder = resource ? this.workspace.getWorkspaceFolder(resource) : undefined;
const systemVariables = new SystemVariables(workspaceFolder ? workspaceFolder.uri.fsPath : undefined);
Expand All @@ -93,22 +118,30 @@ export class InvalidPythonPathInDebuggerService extends BaseDiagnosticsService i
private getCommandPrompts(diagnostic: IDiagnostic): { prompt: string; command?: IDiagnosticCommand }[] {
switch (diagnostic.code) {
case DiagnosticCodes.InvalidPythonPathInDebuggerSettingsDiagnostic: {
return [{
prompt: 'Select Python Interpreter',
command: this.commandFactory.createCommand(diagnostic, { type: 'executeVSCCommand', options: 'python.setInterpreter' })
}];
return [
{
prompt: 'Select Python Interpreter',
command: this.commandFactory.createCommand(diagnostic, {
type: 'executeVSCCommand',
options: 'python.setInterpreter'
})
}
];
}
case DiagnosticCodes.InvalidPythonPathInDebuggerLaunchDiagnostic: {
return [{
prompt: 'Open launch.json',
// tslint:disable-next-line:no-object-literal-type-assertion
command: {
diagnostic, invoke: async (): Promise<void> => {
const launchJson = this.getLaunchJsonFile(workspc.workspaceFolders![0]);
await openFile(launchJson);
return [
{
prompt: 'Open launch.json',
// tslint:disable-next-line:no-object-literal-type-assertion
command: {
diagnostic,
invoke: async (): Promise<void> => {
const launchJson = this.getLaunchJsonFile(workspc.workspaceFolders![0]);
await openFile(launchJson);
}
}
}
}];
];
}
default: {
throw new Error('Invalid diagnostic for \'InvalidPythonPathInDebuggerService\'');
Expand Down
Loading