Skip to content

Modified Select interpreter command & add a reset interpreter command #10373

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

Merged
merged 4 commits into from
Mar 2, 2020
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/1 Enhancements/10372.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Modified `Select interpreter` command to support setting interpreter at workspace level.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this part of this PR? Each PR should cover one change, so shouldn't it be in a separate PR? If it really is integral to the reset interpreter command then I don't see why it should have a separate issue number.

Copy link
Author

@karrtikr karrtikr Mar 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I had to refactor tests for Modified Select interpreter command to support setting interpreter at workspace level while writing tests for Added a reset interpreter command in a similar way.

I can move Added a reset interpreter command in a similar way it into a separate PR but it would still be based on Modified Select interpreter command to support setting interpreter at workspace level PR, so I figured maybe not. Should I do it?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are several advantages to keeping each PR discrete. Some of those advantages:

  • easier to understand
  • easier to identify what changed in the git history
  • easier to revert

At the least I wanted to understand why one PR was addressing 2 distinct issues, before I approved the PR.

Copy link
Author

@karrtikr karrtikr Mar 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand. I should've atleast left a comment explaining the reason I did that. Thanks for the feedback!

1 change: 1 addition & 0 deletions news/1 Enhancements/10374.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Added a command to reset value of Python interpreter.
11 changes: 11 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
"onCommand:python.switchOffInsidersChannel",
"onCommand:python.switchToDailyChannel",
"onCommand:python.switchToWeeklyChannel",
"onCommand:python.resetPythonInterpreter",
"onCommand:python.datascience.createnewnotebook",
"onCommand:python.datascience.showhistorypane",
"onCommand:python.datascience.importnotebook",
Expand Down Expand Up @@ -222,6 +223,11 @@
"title": "%python.command.python.switchToWeeklyChannel.title%",
"category": "Python"
},
{
"command": "python.resetPythonInterpreter",
"title": "%python.command.python.resetPythonInterpreter.title%",
"category": "Python"
},
{
"command": "python.refactorExtractVariable",
"title": "%python.command.python.refactorExtractVariable.title%",
Expand Down Expand Up @@ -746,6 +752,11 @@
"category": "Python",
"when": "config.python.insidersChannel != 'weekly'"
},
{
"command": "python.resetPythonInterpreter",
"title": "%python.command.python.resetPythonInterpreter.title%",
"category": "Python"
},
{
"command": "python.viewOutput",
"title": "%python.command.python.viewOutput.title%",
Expand Down
2 changes: 2 additions & 0 deletions package.nls.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
"python.command.python.switchOffInsidersChannel.title": "Switch to Default Channel",
"python.command.python.switchToDailyChannel.title": "Switch to Insiders Daily Channel",
"python.command.python.switchToWeeklyChannel.title": "Switch to Insiders Weekly Channel",
"python.command.python.resetPythonInterpreter.title": "Reset Python Interpreter",
"python.command.python.refactorExtractVariable.title": "Extract Variable",
"python.command.python.refactorExtractMethod.title": "Extract Method",
"python.command.python.viewOutput.title": "Show Output",
Expand Down Expand Up @@ -131,6 +132,7 @@
"DataScience.connectingToJupyter": "Connecting to Jupyter server",
"Experiments.inGroup": "User belongs to experiment group '{0}'",
"Interpreters.RefreshingInterpreters": "Refreshing Python Interpreters",
"Interpreters.entireWorkspace": "Entire workspace",
"Interpreters.LoadingInterpreters": "Loading Python Interpreters",
"Interpreters.condaInheritEnvMessage": "We noticed you're using a conda environment. If you are experiencing issues with this environment in the integrated terminal, we recommend that you let the Python extension change \"terminal.integrated.inheritEnv\" to false in your user settings.",
"Logging.CurrentWorkingDirectory": "cwd:",
Expand Down
1 change: 1 addition & 0 deletions src/client/common/application/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export type CommandsWithoutArgs = keyof ICommandNameWithoutArgumentTypeMapping;
interface ICommandNameWithoutArgumentTypeMapping {
[Commands.SwitchToInsidersDaily]: [];
[Commands.SwitchToInsidersWeekly]: [];
[Commands.ResetPythonInterpreter]: [];
[Commands.SwitchOffInsidersChannel]: [];
[Commands.Set_Interpreter]: [];
[Commands.Set_ShebangInterpreter]: [];
Expand Down
1 change: 1 addition & 0 deletions src/client/common/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ export namespace Commands {
export const SwitchToInsidersDaily = 'python.switchToDailyChannel';
export const SwitchToInsidersWeekly = 'python.switchToWeeklyChannel';
export const PickLocalProcess = 'python.pickLocalProcess';
export const ResetPythonInterpreter = 'python.resetPythonInterpreter';
}
export namespace Octicons {
export const Test_Pass = '$(check)';
Expand Down
1 change: 1 addition & 0 deletions src/client/common/utils/localize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ export namespace Interpreters {
'Interpreters.environmentPromptMessage',
'We noticed a new virtual environment has been created. Do you want to select it for the workspace folder?'
);
export const entireWorkspace = localize('Interpreters.entireWorkspace', 'Entire workspace');
export const selectInterpreterTip = localize(
'Interpreters.selectInterpreterTip',
'Tip: you can change the Python interpreter used by the Python extension by clicking on the Python version in the status bar'
Expand Down
75 changes: 56 additions & 19 deletions src/client/interpreter/configuration/interpreterSelector.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { inject, injectable } from 'inversify';
import { ConfigurationTarget, Disposable, QuickPickOptions, Uri } from 'vscode';
import * as path from 'path';
import { ConfigurationTarget, Disposable, QuickPickItem, QuickPickOptions, Uri } from 'vscode';
import {
IApplicationShell,
ICommandManager,
Expand All @@ -8,7 +9,8 @@ import {
} from '../../common/application/types';
import { Commands } from '../../common/constants';
import { IConfigurationService, IPathUtils, Resource } from '../../common/types';
import { IInterpreterService, IShebangCodeLensProvider, PythonInterpreter, WorkspacePythonPath } from '../contracts';
import { Interpreters } from '../../common/utils/localize';
import { IInterpreterService, IShebangCodeLensProvider, PythonInterpreter } from '../contracts';
import {
IInterpreterComparer,
IInterpreterQuickPickItem,
Expand Down Expand Up @@ -41,6 +43,9 @@ export class InterpreterSelector implements IInterpreterSelector {
this.disposables.push(
this.commandManager.registerCommand(Commands.Set_Interpreter, this.setInterpreter.bind(this))
);
this.disposables.push(
this.commandManager.registerCommand(Commands.ResetPythonInterpreter, this.resetInterpreter.bind(this))
);
this.disposables.push(
this.commandManager.registerCommand(Commands.Set_ShebangInterpreter, this.setShebangInterpreter.bind(this))
);
Expand All @@ -51,6 +56,17 @@ export class InterpreterSelector implements IInterpreterSelector {
interpreters.sort(this.interpreterComparer.compare.bind(this.interpreterComparer));
return Promise.all(interpreters.map(item => this.suggestionToQuickPickItem(item, resource)));
}

protected async resetInterpreter() {
const targetConfig = await this.getConfigTarget();
if (!targetConfig) {
return;
}
const configTarget = targetConfig.configTarget;
const wkspace = targetConfig.folderUri;

await this.pythonPathUpdaterService.updatePythonPath(undefined, configTarget, 'ui', wkspace);
}
protected async suggestionToQuickPickItem(
suggestion: PythonInterpreter,
workspaceUri?: Uri
Expand All @@ -67,19 +83,12 @@ export class InterpreterSelector implements IInterpreterSelector {
}

protected async setInterpreter() {
const setInterpreterGlobally =
!Array.isArray(this.workspaceService.workspaceFolders) ||
this.workspaceService.workspaceFolders.length === 0;
let configTarget = ConfigurationTarget.Global;
let wkspace: Uri | undefined;
if (!setInterpreterGlobally) {
const targetConfig = await this.getWorkspaceToSetPythonPath();
if (!targetConfig) {
return;
}
configTarget = targetConfig.configTarget;
wkspace = targetConfig.folderUri;
const targetConfig = await this.getConfigTarget();
if (!targetConfig) {
return;
}
const configTarget = targetConfig.configTarget;
const wkspace = targetConfig.folderUri;

const suggestions = await this.getSuggestions(wkspace);
const currentPythonPath = this.pathUtils.getDisplayName(
Expand Down Expand Up @@ -138,12 +147,21 @@ export class InterpreterSelector implements IInterpreterSelector {
workspaceFolder.uri
);
}
private async getWorkspaceToSetPythonPath(): Promise<WorkspacePythonPath | undefined> {
private async getConfigTarget(): Promise<
| {
folderUri: Resource;
configTarget: ConfigurationTarget;
}
| undefined
> {
if (
!Array.isArray(this.workspaceService.workspaceFolders) ||
this.workspaceService.workspaceFolders.length === 0
) {
return undefined;
return {
folderUri: undefined,
configTarget: ConfigurationTarget.Global
};
}
if (this.workspaceService.workspaceFolders.length === 1) {
return {
Expand All @@ -153,11 +171,30 @@ export class InterpreterSelector implements IInterpreterSelector {
}

// Ok we have multiple workspaces, get the user to pick a folder.
const workspaceFolder = await this.applicationShell.showWorkspaceFolderPick({

type WorkspaceSelectionQuickPickItem = QuickPickItem & { uri: Uri };
const quickPickItems: WorkspaceSelectionQuickPickItem[] = [
...this.workspaceService.workspaceFolders.map(w => {
({
label: w.name,
description: path.dirname(w.uri.fsPath),
uri: w.uri
})
}),
{
label: Interpreters.entireWorkspace(),
uri: this.workspaceService.workspaceFolders[0].uri
}
];

const selection = await this.applicationShell.showQuickPick(quickPickItems, {
placeHolder: 'Select the workspace to set the interpreter'
});
return workspaceFolder
? { folderUri: workspaceFolder.uri, configTarget: ConfigurationTarget.WorkspaceFolder }

return selection
? selection.label === Interpreters.entireWorkspace()
? { folderUri: selection.uri, configTarget: ConfigurationTarget.Workspace }
: { folderUri: selection.uri, configTarget: ConfigurationTarget.WorkspaceFolder }
: undefined;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export class PythonPathUpdaterService implements IPythonPathUpdaterServiceManage
this.executionFactory = serviceContainer.get<IPythonExecutionFactory>(IPythonExecutionFactory);
}
public async updatePythonPath(
pythonPath: string,
pythonPath: string | undefined,
configTarget: ConfigurationTarget,
trigger: 'ui' | 'shebang' | 'load',
wkspace?: Uri
Expand All @@ -33,7 +33,7 @@ export class PythonPathUpdaterService implements IPythonPathUpdaterServiceManage
const pythonPathUpdater = this.getPythonUpdaterService(configTarget, wkspace);
let failed = false;
try {
await pythonPathUpdater.updatePythonPath(path.normalize(pythonPath));
await pythonPathUpdater.updatePythonPath(pythonPath ? path.normalize(pythonPath) : undefined);
} catch (reason) {
failed = true;
// tslint:disable-next-line:no-unsafe-any prefer-type-cast
Expand All @@ -50,13 +50,13 @@ export class PythonPathUpdaterService implements IPythonPathUpdaterServiceManage
duration: number,
failed: boolean,
trigger: 'ui' | 'shebang' | 'load',
pythonPath: string
pythonPath: string | undefined
) {
const telemtryProperties: PythonInterpreterTelemetry = {
failed,
trigger
};
if (!failed) {
if (!failed && pythonPath) {
const processService = await this.executionFactory.create({ pythonPath });
const infoPromise = processService
.getInterpreterInformation()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { IPythonPathUpdaterService } from '../types';

export class GlobalPythonPathUpdaterService implements IPythonPathUpdaterService {
constructor(private readonly workspaceService: IWorkspaceService) {}
public async updatePythonPath(pythonPath: string): Promise<void> {
public async updatePythonPath(pythonPath: string | undefined): Promise<void> {
const pythonConfig = this.workspaceService.getConfiguration('python');
const pythonPathValue = pythonConfig.inspect<string>('pythonPath');

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@ import { IPythonPathUpdaterService } from '../types';

export class WorkspaceFolderPythonPathUpdaterService implements IPythonPathUpdaterService {
constructor(private workspaceFolder: Uri, private readonly workspaceService: IWorkspaceService) {}
public async updatePythonPath(pythonPath: string): Promise<void> {
public async updatePythonPath(pythonPath: string | undefined): Promise<void> {
const pythonConfig = this.workspaceService.getConfiguration('python', this.workspaceFolder);
const pythonPathValue = pythonConfig.inspect<string>('pythonPath');

if (pythonPathValue && pythonPathValue.workspaceFolderValue === pythonPath) {
return;
}
if (pythonPath.startsWith(this.workspaceFolder.fsPath)) {
if (pythonPath && pythonPath.startsWith(this.workspaceFolder.fsPath)) {
pythonPath = path.relative(this.workspaceFolder.fsPath, pythonPath);
}
await pythonConfig.update('pythonPath', pythonPath, ConfigurationTarget.WorkspaceFolder);
Expand Down
4 changes: 2 additions & 2 deletions src/client/interpreter/configuration/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { Resource } from '../../common/types';
import { PythonInterpreter } from '../contracts';

export interface IPythonPathUpdaterService {
updatePythonPath(pythonPath: string): Promise<void>;
updatePythonPath(pythonPath: string | undefined): Promise<void>;
}

export const IPythonPathUpdaterServiceFactory = Symbol('IPythonPathUpdaterServiceFactory');
Expand All @@ -16,7 +16,7 @@ export interface IPythonPathUpdaterServiceFactory {
export const IPythonPathUpdaterServiceManager = Symbol('IPythonPathUpdaterServiceManager');
export interface IPythonPathUpdaterServiceManager {
updatePythonPath(
pythonPath: string,
pythonPath: string | undefined,
configTarget: ConfigurationTarget,
trigger: 'ui' | 'shebang' | 'load',
wkspace?: Uri
Expand Down
Loading