Skip to content

Ensure extension features are started when in Deprecate PythonPath experiment and opening a file without any folder opened #12182

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

Merged
merged 2 commits into from
Jun 8, 2020
Merged

Ensure extension features are started when in Deprecate PythonPath experiment and opening a file without any folder opened #12182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
61a1e7f
Ensure extension features are started when in Deprecate PythonPath ex…
Jun 7, 2020
208db83
Added comments
Jun 7, 2020
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/12177.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Ensure extension features are started when in `Deprecate PythonPath` experiment and opening a file without any folder opened.
10 changes: 8 additions & 2 deletions src/client/common/interpreterPathService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { ConfigurationChangeEvent, ConfigurationTarget, Event, EventEmitter, Uri
import { IWorkspaceService } from './application/types';
import { PythonSettings } from './configSettings';
import { isTestExecution } from './constants';
import { traceError } from './logger';
import { FileSystemPaths } from './platform/fs-paths';
import {
IDisposable,
Expand Down Expand Up @@ -105,7 +106,8 @@ export class InterpreterPathService implements IInterpreterPathService {
return;
}
if (!resource) {
throw new Error('Cannot update workspace settings as no workspace is opened');
traceError('Cannot update workspace settings as no workspace is opened');
return;
}
const settingKey = this.getSettingKey(resource, configTarget);
const persistentSetting = this.persistentStateFactory.createGlobalPersistentState<string | undefined>(
Expand Down Expand Up @@ -149,7 +151,11 @@ export class InterpreterPathService implements IInterpreterPathService {

public async _copyWorkspaceFolderValueToNewStorage(resource: Resource, value: string | undefined): Promise<void> {
// Copy workspace folder setting into the new storage if it hasn't been copied already
const workspaceFolderKey = this.workspaceService.getWorkspaceFolderIdentifier(resource);
const workspaceFolderKey = this.workspaceService.getWorkspaceFolderIdentifier(resource, '');
if (workspaceFolderKey === '') {
// No workspace folder is opened, simply return.
return;
}
const flaggedWorkspaceFolderKeysStorage = this.persistentStateFactory.createGlobalPersistentState<string[]>(
workspaceFolderKeysForWhichTheCopyIsDone_Key,
[]
Expand Down
39 changes: 23 additions & 16 deletions src/test/common/interpreterPathService.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ suite('Interpreter Path Service', async () => {
test('If the one-off transfer to new storage has not happened yet for the workspace folder, do it and record the transfer', async () => {
const update = sinon.stub(InterpreterPathService.prototype, 'update');
const persistentState = TypeMoq.Mock.ofType<IPersistentState<string[]>>();
workspaceService.setup((w) => w.getWorkspaceFolderIdentifier(resource)).returns(() => resource.fsPath);
workspaceService.setup((w) => w.getWorkspaceFolderIdentifier(resource, '')).returns(() => resource.fsPath);
persistentStateFactory
.setup((p) => p.createGlobalPersistentState<string[]>(workspaceFolderKeysForWhichTheCopyIsDone_Key, []))
.returns(() => persistentState.object);
Expand All @@ -112,7 +112,7 @@ suite('Interpreter Path Service', async () => {
test('If the one-off transfer to new storage has already happened for the workspace folder, do not update and simply return', async () => {
const update = sinon.stub(InterpreterPathService.prototype, 'update');
const persistentState = TypeMoq.Mock.ofType<IPersistentState<string[]>>();
workspaceService.setup((w) => w.getWorkspaceFolderIdentifier(resource)).returns(() => resource.fsPath);
workspaceService.setup((w) => w.getWorkspaceFolderIdentifier(resource, '')).returns(() => resource.fsPath);
persistentStateFactory
.setup((p) => p.createGlobalPersistentState<string[]>(workspaceFolderKeysForWhichTheCopyIsDone_Key, []))
.returns(() => persistentState.object);
Expand All @@ -126,6 +126,23 @@ suite('Interpreter Path Service', async () => {
persistentState.verifyAll();
});

test('If no folder is opened, do not do the one-off transfer to new storage for the workspace folder', async () => {
const update = sinon.stub(InterpreterPathService.prototype, 'update');
const persistentState = TypeMoq.Mock.ofType<IPersistentState<string[]>>();
workspaceService.setup((w) => w.getWorkspaceFolderIdentifier(resource, '')).returns(() => '');
persistentStateFactory
.setup((p) => p.createGlobalPersistentState<string[]>(workspaceFolderKeysForWhichTheCopyIsDone_Key, []))
.returns(() => persistentState.object);
persistentState.setup((p) => p.value).returns(() => ['...storedWorkspaceKeys']);
persistentState.setup((p) => p.updateValue(TypeMoq.It.isAny())).verifiable(TypeMoq.Times.never());

interpreterPathService = new InterpreterPathService(persistentStateFactory.object, workspaceService.object, []);
await interpreterPathService._copyWorkspaceFolderValueToNewStorage(resource, 'workspaceFolderPythonPath');

assert(update.notCalled);
persistentState.verifyAll();
});

test('If the one-off transfer to new storage has not happened yet for the workspace, do it and record the transfer', async () => {
const workspaceFileUri = Uri.parse('path/to/workspaceFile');
const expectedWorkspaceKey = fs.normCase(workspaceFileUri.fsPath);
Expand Down Expand Up @@ -395,7 +412,7 @@ suite('Interpreter Path Service', async () => {
_didChangeInterpreterEmitter.verifyAll();
});

test('Updating workspace settings throws error if no workspace is opened', async () => {
test('Updating workspace settings simply returns if no workspace is opened', async () => {
const expectedSettingKey = `WORKSPACE_FOLDER_INTERPRETER_PATH_${resource.fsPath}`;
const persistentState = TypeMoq.Mock.ofType<IPersistentState<string | undefined>>();
workspaceService.setup((w) => w.workspaceFolders).returns(() => undefined);
Expand All @@ -408,18 +425,13 @@ suite('Interpreter Path Service', async () => {
.returns(() => Promise.resolve())
.verifiable(TypeMoq.Times.never());

const promise = interpreterPathService.update(
resourceOutsideOfWorkspace,
ConfigurationTarget.Workspace,
interpreterPath
);
await expect(promise).to.eventually.be.rejectedWith(Error);
await interpreterPathService.update(resourceOutsideOfWorkspace, ConfigurationTarget.Workspace, interpreterPath);

persistentState.verifyAll();
persistentStateFactory.verifyAll();
});

test('Updating workspace folder settings throws error if no workspace is opened', async () => {
test('Updating workspace folder settings simply returns if no workspace is opened', async () => {
const expectedSettingKey = `WORKSPACE_FOLDER_INTERPRETER_PATH_${resource.fsPath}`;
const persistentState = TypeMoq.Mock.ofType<IPersistentState<string | undefined>>();
workspaceService.setup((w) => w.workspaceFolders).returns(() => undefined);
Expand All @@ -432,12 +444,7 @@ suite('Interpreter Path Service', async () => {
.returns(() => Promise.resolve())
.verifiable(TypeMoq.Times.never());

const promise = interpreterPathService.update(
resourceOutsideOfWorkspace,
ConfigurationTarget.Workspace,
interpreterPath
);
await expect(promise).to.eventually.be.rejectedWith(Error);
await interpreterPathService.update(resourceOutsideOfWorkspace, ConfigurationTarget.Workspace, interpreterPath);

persistentState.verifyAll();
persistentStateFactory.verifyAll();
Expand Down