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.

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

Ensure we block on autoselection when no interpreter is explictly set by user #16723

Merged
merged 3 commits into from
Jul 20, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions news/2 Fixes/16723.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Ensure we block on autoselection when no interpreter is explictly set by user.
33 changes: 33 additions & 0 deletions src/client/common/interpreterPathProxyService.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

'use strict';

import { inject, injectable } from 'inversify';
import { IWorkspaceService } from './application/types';
import { DeprecatePythonPath } from './experiments/groups';
import { IExperimentService, IInterpreterPathProxyService, IInterpreterPathService, Resource } from './types';
import { SystemVariables } from './variables/systemVariables';

@injectable()
export class InterpreterPathProxyService implements IInterpreterPathProxyService {
constructor(
@inject(IInterpreterPathService) private readonly interpreterPathService: IInterpreterPathService,
@inject(IExperimentService) private readonly experiment: IExperimentService,
@inject(IWorkspaceService) private readonly workspace: IWorkspaceService,
) {}

public get(resource: Resource): string {
const systemVariables = new SystemVariables(
undefined,
this.workspace.getWorkspaceFolder(resource)?.uri.fsPath,
this.workspace,
);
const pythonSettings = this.workspace.getConfiguration('python', resource);
return systemVariables.resolveAny(
this.experiment.inExperimentSync(DeprecatePythonPath.experiment)
? this.interpreterPathService.get(resource)
: pythonSettings.get<string>('pythonPath'),
)!;
}
}
6 changes: 6 additions & 0 deletions src/client/common/serviceRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
IToolExecutionPath,
IsWindows,
ToolExecutionPath,
IInterpreterPathProxyService,
} from './types';
import { IServiceManager } from '../ioc/types';
import { JupyterExtensionDependencyManager } from '../jupyter/jupyterExtensionDependencyManager';
Expand Down Expand Up @@ -118,11 +119,16 @@ import { IMultiStepInputFactory, MultiStepInputFactory } from './utils/multiStep
import { Random } from './utils/random';
import { JupyterNotInstalledNotificationHelper } from '../jupyter/jupyterNotInstalledNotificationHelper';
import { IJupyterNotInstalledNotificationHelper } from '../jupyter/types';
import { InterpreterPathProxyService } from './interpreterPathProxyService';

export function registerTypes(serviceManager: IServiceManager): void {
serviceManager.addSingletonInstance<boolean>(IsWindows, IS_WINDOWS);

serviceManager.addSingleton<IActiveResourceService>(IActiveResourceService, ActiveResourceService);
serviceManager.addSingleton<IInterpreterPathProxyService>(
IInterpreterPathProxyService,
InterpreterPathProxyService,
);
serviceManager.addSingleton<IInterpreterPathService>(IInterpreterPathService, InterpreterPathService);
serviceManager.addSingleton<IExtensions>(IExtensions, Extensions);
serviceManager.addSingleton<IRandom>(IRandom, Random);
Expand Down
8 changes: 8 additions & 0 deletions src/client/common/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -557,6 +557,14 @@ export interface IInterpreterPathService {
copyOldInterpreterStorageValuesToNew(resource: Uri | undefined): Promise<void>;
}

/**
* Interface used to access current Interpreter Path
*/
export const IInterpreterPathProxyService = Symbol('IInterpreterPathProxyService');
export interface IInterpreterPathProxyService {
get(resource: Resource): string;
}

export type DefaultLSType = LanguageServerType.Jedi | LanguageServerType.JediLSP | LanguageServerType.Node;

/**
Expand Down
16 changes: 12 additions & 4 deletions src/client/interpreter/display/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,13 @@ import { Disposable, OutputChannel, StatusBarAlignment, StatusBarItem, Uri } fro
import { IApplicationShell, IWorkspaceService } from '../../common/application/types';
import { STANDARD_OUTPUT_CHANNEL } from '../../common/constants';
import '../../common/extensions';
import { IConfigurationService, IDisposableRegistry, IOutputChannel, IPathUtils, Resource } from '../../common/types';
import {
IDisposableRegistry,
IInterpreterPathProxyService,
IOutputChannel,
IPathUtils,
Resource,
} from '../../common/types';
import { Interpreters } from '../../common/utils/localize';
import { IServiceContainer } from '../../ioc/types';
import { PythonEnvironment } from '../../pythonEnvironments/info';
Expand All @@ -22,7 +28,7 @@ export class InterpreterDisplay implements IInterpreterDisplay {
private readonly workspaceService: IWorkspaceService;
private readonly pathUtils: IPathUtils;
private readonly interpreterService: IInterpreterService;
private readonly configService: IConfigurationService;
private readonly interpreterPathExpHelper: IInterpreterPathProxyService;
private currentlySelectedInterpreterPath?: string;
private currentlySelectedWorkspaceFolder: Resource;
private readonly autoSelection: IInterpreterAutoSelectionService;
Expand All @@ -39,7 +45,9 @@ export class InterpreterDisplay implements IInterpreterDisplay {

const application = serviceContainer.get<IApplicationShell>(IApplicationShell);
const disposableRegistry = serviceContainer.get<Disposable[]>(IDisposableRegistry);
this.configService = serviceContainer.get<IConfigurationService>(IConfigurationService);
this.interpreterPathExpHelper = serviceContainer.get<IInterpreterPathProxyService>(
IInterpreterPathProxyService,
);

this.statusBar = application.createStatusBarItem(StatusBarAlignment.Left, 100);
this.statusBar.command = 'python.setInterpreter';
Expand Down Expand Up @@ -75,7 +83,7 @@ export class InterpreterDisplay implements IInterpreterDisplay {
}
}
private async updateDisplay(workspaceFolder?: Uri) {
const interpreterPath = this.configService.getSettings(workspaceFolder)?.pythonPath;
const interpreterPath = this.interpreterPathExpHelper.get(workspaceFolder);
if (!interpreterPath || interpreterPath === 'python') {
await this.autoSelection.autoSelectInterpreter(workspaceFolder); // Block on this only if no interpreter selected.
}
Expand Down
54 changes: 54 additions & 0 deletions src/test/common/interpreterPathProxyService.unit.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

'use strict';

import { Uri, WorkspaceConfiguration } from 'vscode';
import * as TypeMoq from 'typemoq';
import { expect } from 'chai';
import { InterpreterPathProxyService } from '../../client/common/interpreterPathProxyService';
import { IExperimentService, IInterpreterPathProxyService, IInterpreterPathService } from '../../client/common/types';
import { IWorkspaceService } from '../../client/common/application/types';
import { DeprecatePythonPath } from '../../client/common/experiments/groups';

suite('Interpreter Path Proxy Service', async () => {
let interpreterPathProxyService: IInterpreterPathProxyService;
let workspaceService: TypeMoq.IMock<IWorkspaceService>;
let experiments: TypeMoq.IMock<IExperimentService>;
let interpreterPathService: TypeMoq.IMock<IInterpreterPathService>;
const resource = Uri.parse('a');
const interpreterPath = 'path/to/interpreter';
setup(() => {
workspaceService = TypeMoq.Mock.ofType<IWorkspaceService>();
experiments = TypeMoq.Mock.ofType<IExperimentService>();
interpreterPathService = TypeMoq.Mock.ofType<IInterpreterPathService>();
workspaceService
.setup((w) => w.getWorkspaceFolder(resource))
.returns(() => ({
uri: resource,
name: 'Workspacefolder',
index: 0,
}));
interpreterPathProxyService = new InterpreterPathProxyService(
interpreterPathService.object,
experiments.object,
workspaceService.object,
);
});

test('When in experiment, use interpreter path service to get setting value', () => {
experiments.setup((e) => e.inExperimentSync(DeprecatePythonPath.experiment)).returns(() => true);
interpreterPathService.setup((i) => i.get(resource)).returns(() => interpreterPath);
const value = interpreterPathProxyService.get(resource);
expect(value).to.equal(interpreterPath);
});

test('When not in experiment, use workspace service to get setting value', () => {
experiments.setup((e) => e.inExperimentSync(DeprecatePythonPath.experiment)).returns(() => false);
const workspaceConfig = TypeMoq.Mock.ofType<WorkspaceConfiguration>();
workspaceService.setup((i) => i.getConfiguration('python', resource)).returns(() => workspaceConfig.object);
workspaceConfig.setup((w) => w.get('pythonPath')).returns(() => interpreterPath);
const value = interpreterPathProxyService.get(resource);
expect(value).to.equal(interpreterPath);
});
});
16 changes: 6 additions & 10 deletions src/test/interpreters/display.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,10 @@ import { IApplicationShell, IWorkspaceService } from '../../client/common/applic
import { STANDARD_OUTPUT_CHANNEL } from '../../client/common/constants';
import { IFileSystem } from '../../client/common/platform/types';
import {
IConfigurationService,
IInterpreterPathProxyService,
IDisposableRegistry,
IOutputChannel,
IPathUtils,
IPythonSettings,
ReadWrite,
} from '../../client/common/types';
import { Interpreters } from '../../client/common/utils/localize';
Expand Down Expand Up @@ -57,8 +56,7 @@ suite('Interpreters Display', () => {
let fileSystem: TypeMoq.IMock<IFileSystem>;
let disposableRegistry: Disposable[];
let statusBar: TypeMoq.IMock<StatusBarItem>;
let pythonSettings: TypeMoq.IMock<IPythonSettings>;
let configurationService: TypeMoq.IMock<IConfigurationService>;
let interpreterPathProxyService: TypeMoq.IMock<IInterpreterPathProxyService>;
let interpreterDisplay: IInterpreterDisplay;
let interpreterHelper: TypeMoq.IMock<IInterpreterHelper>;
let pathUtils: TypeMoq.IMock<IPathUtils>;
Expand All @@ -73,8 +71,7 @@ suite('Interpreters Display', () => {
interpreterHelper = TypeMoq.Mock.ofType<IInterpreterHelper>();
disposableRegistry = [];
statusBar = TypeMoq.Mock.ofType<StatusBarItem>();
pythonSettings = TypeMoq.Mock.ofType<IPythonSettings>();
configurationService = TypeMoq.Mock.ofType<IConfigurationService>();
interpreterPathProxyService = TypeMoq.Mock.ofType<IInterpreterPathProxyService>();
pathUtils = TypeMoq.Mock.ofType<IPathUtils>();
output = TypeMoq.Mock.ofType<IOutputChannel>();
autoSelection = mock(InterpreterAutoSelectionService);
Expand All @@ -94,8 +91,8 @@ suite('Interpreters Display', () => {
serviceContainer.setup((c) => c.get(TypeMoq.It.isValue(IFileSystem))).returns(() => fileSystem.object);
serviceContainer.setup((c) => c.get(TypeMoq.It.isValue(IDisposableRegistry))).returns(() => disposableRegistry);
serviceContainer
.setup((c) => c.get(TypeMoq.It.isValue(IConfigurationService)))
.returns(() => configurationService.object);
.setup((c) => c.get(TypeMoq.It.isValue(IInterpreterPathProxyService)))
.returns(() => interpreterPathProxyService.object);
serviceContainer
.setup((c) => c.get(TypeMoq.It.isValue(IInterpreterHelper)))
.returns(() => interpreterHelper.object);
Expand Down Expand Up @@ -215,8 +212,7 @@ suite('Interpreters Display', () => {
interpreterService
.setup((i) => i.getActiveInterpreter(TypeMoq.It.isValue(workspaceFolder)))
.returns(() => Promise.resolve(undefined));
configurationService.setup((c) => c.getSettings(TypeMoq.It.isAny())).returns(() => pythonSettings.object);
pythonSettings.setup((p) => p.pythonPath).returns(() => pythonPath);
interpreterPathProxyService.setup((c) => c.get(TypeMoq.It.isAny())).returns(() => pythonPath);
fileSystem.setup((f) => f.fileExists(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve(false));
interpreterHelper
.setup((v) => v.getInterpreterInformation(TypeMoq.It.isValue(pythonPath)))
Expand Down