Skip to content

chore: refactoring and cleanup #337

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 1 commit into from
Apr 25, 2025
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
10 changes: 5 additions & 5 deletions src/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,16 @@
// Licensed under the MIT License.

import {
Uri,
Disposable,
MarkdownString,
Event,
FileChangeType,
LogOutputChannel,
ThemeIcon,
Terminal,
MarkdownString,
TaskExecution,
Terminal,
TerminalOptions,
FileChangeType,
ThemeIcon,
Uri,
} from 'vscode';

/**
Expand Down
3 changes: 2 additions & 1 deletion src/common/localize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,10 +165,11 @@ export namespace EnvViewStrings {
export const selectedWorkspaceTooltip = l10n.t('This environment is selected for workspace files');
}

export namespace ShellStartupActivationStrings {
export namespace ActivationStrings {
export const envCollectionDescription = l10n.t('Environment variables for shell activation');
export const revertedShellStartupScripts = l10n.t(
'Removed shell startup profile code for Python environment activation. See [logs](command:{0})',
Commands.viewLogs,
);
export const activatingEnvironment = l10n.t('Activating environment');
}
23 changes: 11 additions & 12 deletions src/features/terminal/shellStartupActivationVariablesManager.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import { ConfigurationChangeEvent, Disposable, GlobalEnvironmentVariableCollection } from 'vscode';
import { DidChangeEnvironmentEventArgs } from '../../api';
import { ShellStartupActivationStrings } from '../../common/localize';
import { DidChangeEnvironmentEventArgs, PythonProjectEnvironmentApi } from '../../api';
import { ActivationStrings } from '../../common/localize';
import { getWorkspaceFolder, getWorkspaceFolders, onDidChangeConfiguration } from '../../common/workspace.apis';
import { EnvironmentManagers } from '../../internal.api';
import { ShellEnvsProvider } from './shells/startupProvider';
import { getAutoActivationType } from './utils';
import { ACT_TYPE_SHELL, getAutoActivationType } from './utils';

export interface ShellStartupActivationVariablesManager extends Disposable {
initialize(): Promise<void>;
Expand All @@ -15,14 +14,14 @@ export class ShellStartupActivationVariablesManagerImpl implements ShellStartupA
constructor(
private readonly envCollection: GlobalEnvironmentVariableCollection,
private readonly shellEnvsProviders: ShellEnvsProvider[],
private readonly em: EnvironmentManagers,
private readonly api: PythonProjectEnvironmentApi,
) {
this.envCollection.description = ShellStartupActivationStrings.envCollectionDescription;
this.envCollection.description = ActivationStrings.envCollectionDescription;
this.disposables.push(
onDidChangeConfiguration(async (e: ConfigurationChangeEvent) => {
await this.handleConfigurationChange(e);
}),
this.em.onDidChangeEnvironmentFiltered(async (e: DidChangeEnvironmentEventArgs) => {
this.api.onDidChangeEnvironment(async (e: DidChangeEnvironmentEventArgs) => {
await this.handleEnvironmentChange(e);
}),
);
Expand All @@ -31,7 +30,7 @@ export class ShellStartupActivationVariablesManagerImpl implements ShellStartupA
private async handleConfigurationChange(e: ConfigurationChangeEvent) {
if (e.affectsConfiguration('python-envs.terminal.autoActivationType')) {
const autoActType = getAutoActivationType();
if (autoActType === 'shellStartup') {
if (autoActType === ACT_TYPE_SHELL) {
await this.initializeInternal();
} else {
const workspaces = getWorkspaceFolders() ?? [];
Expand All @@ -49,7 +48,7 @@ export class ShellStartupActivationVariablesManagerImpl implements ShellStartupA

private async handleEnvironmentChange(e: DidChangeEnvironmentEventArgs) {
const autoActType = getAutoActivationType();
if (autoActType === 'shellStartup' && e.uri) {
if (autoActType === ACT_TYPE_SHELL && e.uri) {
const wf = getWorkspaceFolder(e.uri);
if (wf) {
const envVars = this.envCollection.getScoped({ workspaceFolder: wf });
Expand All @@ -75,7 +74,7 @@ export class ShellStartupActivationVariablesManagerImpl implements ShellStartupA
const collection = this.envCollection.getScoped({ workspaceFolder: workspace });
promises.push(
...this.shellEnvsProviders.map(async (provider) => {
const env = await this.em.getEnvironment(workspace.uri);
const env = await this.api.getEnvironment(workspace.uri);
if (env) {
provider.updateEnvVariables(collection, env);
} else {
Expand All @@ -86,7 +85,7 @@ export class ShellStartupActivationVariablesManagerImpl implements ShellStartupA
});
await Promise.all(promises);
} else {
const env = await this.em.getEnvironment(undefined);
const env = await this.api.getEnvironment(undefined);
await Promise.all(
this.shellEnvsProviders.map(async (provider) => {
if (env) {
Expand All @@ -101,7 +100,7 @@ export class ShellStartupActivationVariablesManagerImpl implements ShellStartupA

public async initialize(): Promise<void> {
const autoActType = getAutoActivationType();
if (autoActType === 'shellStartup') {
if (autoActType === ACT_TYPE_SHELL) {
await this.initializeInternal();
}
}
Expand Down
12 changes: 6 additions & 6 deletions src/features/terminal/shellStartupSetupHandlers.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { l10n, ProgressLocation } from 'vscode';
import { executeCommand } from '../../common/command.api';
import { Common, ShellStartupActivationStrings } from '../../common/localize';
import { ActivationStrings, Common } from '../../common/localize';
import { traceInfo, traceVerbose } from '../../common/logging';
import { showErrorMessage, showInformationMessage, withProgress } from '../../common/window.apis';
import { ShellScriptEditState, ShellStartupScriptProvider } from './shells/startupProvider';
import { getAutoActivationType, setAutoActivationType } from './utils';
import { ACT_TYPE_COMMAND, ACT_TYPE_SHELL, getAutoActivationType, setAutoActivationType } from './utils';

export async function handleSettingUpShellProfile(
providers: ShellStartupScriptProvider[],
Expand All @@ -14,7 +14,7 @@ export async function handleSettingUpShellProfile(
const response = await showInformationMessage(
l10n.t(
'To use "{0}" activation, the shell profiles need to be set up. Do you want to set it up now?',
'shellStartup',
ACT_TYPE_SHELL,
),
{ modal: true, detail: l10n.t('Shells: {0}', shells) },
Common.yes,
Expand Down Expand Up @@ -59,11 +59,11 @@ export async function handleSettingUpShellProfile(

export async function cleanupStartupScripts(allProviders: ShellStartupScriptProvider[]): Promise<void> {
await Promise.all(allProviders.map((provider) => provider.teardownScripts()));
if (getAutoActivationType() === 'shellStartup') {
setAutoActivationType('command');
if (getAutoActivationType() === ACT_TYPE_SHELL) {
setAutoActivationType(ACT_TYPE_COMMAND);
traceInfo(
'Setting `python-envs.terminal.autoActivationType` to `command`, after removing shell startup scripts.',
);
}
setImmediate(async () => await showInformationMessage(ShellStartupActivationStrings.revertedShellStartupScripts));
setImmediate(async () => await showInformationMessage(ActivationStrings.revertedShellStartupScripts));
}
40 changes: 25 additions & 15 deletions src/features/terminal/terminalManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import * as fsapi from 'fs-extra';
import * as path from 'path';
import { Disposable, EventEmitter, ProgressLocation, Terminal, TerminalOptions, Uri } from 'vscode';
import { PythonEnvironment, PythonEnvironmentApi, PythonProject, PythonTerminalCreateOptions } from '../../api';
import { ActivationStrings } from '../../common/localize';
import { traceInfo, traceVerbose } from '../../common/logging';
import {
createTerminal,
Expand All @@ -23,7 +24,15 @@ import {
TerminalActivationInternal,
TerminalEnvironment,
} from './terminalActivationState';
import { AutoActivationType, getAutoActivationType, getEnvironmentForTerminal, waitForShellIntegration } from './utils';
import {
ACT_TYPE_COMMAND,
ACT_TYPE_OFF,
ACT_TYPE_SHELL,
AutoActivationType,
getAutoActivationType,
getEnvironmentForTerminal,
waitForShellIntegration,
} from './utils';

export interface TerminalCreation {
create(environment: PythonEnvironment, options: PythonTerminalCreateOptions): Promise<Terminal>;
Expand Down Expand Up @@ -108,7 +117,7 @@ export class TerminalManagerImpl implements TerminalManager {
onDidChangeConfiguration(async (e) => {
if (e.affectsConfiguration('python-envs.terminal.autoActivationType')) {
const actType = getAutoActivationType();
if (actType === 'shellStartup') {
if (actType === ACT_TYPE_SHELL) {
traceInfo(`Auto activation type changed to ${actType}`);
const shells = new Set(
terminals()
Expand Down Expand Up @@ -178,10 +187,10 @@ export class TerminalManagerImpl implements TerminalManager {
let isSetup = this.shellSetup.get(shellType);
if (isSetup === true) {
traceVerbose(`Shell profile for ${shellType} is already setup.`);
return 'shellStartup';
return ACT_TYPE_SHELL;
} else if (isSetup === false) {
traceVerbose(`Shell profile for ${shellType} is not set up, using command fallback.`);
return 'command';
return ACT_TYPE_COMMAND;
}
}

Expand All @@ -197,25 +206,25 @@ export class TerminalManagerImpl implements TerminalManager {
await this.handleSetupCheck(shellType);

// Check again after the setup check.
return this.getShellActivationType(shellType) ?? 'command';
return this.getShellActivationType(shellType) ?? ACT_TYPE_COMMAND;
}
traceInfo(`Shell startup not supported for ${shellType}, using command activation as fallback`);
return 'command';
return ACT_TYPE_COMMAND;
}

private async autoActivateOnTerminalOpen(terminal: Terminal, environment: PythonEnvironment): Promise<void> {
let actType = getAutoActivationType();
const shellType = identifyTerminalShell(terminal);
if (actType === 'shellStartup') {
if (actType === ACT_TYPE_SHELL) {
actType = await this.getEffectiveActivationType(shellType);
}

if (actType === 'command') {
if (actType === ACT_TYPE_COMMAND) {
if (isActivatableEnvironment(environment)) {
await withProgress(
{
location: ProgressLocation.Window,
title: `Activating environment: ${environment.environmentPath.fsPath}`,
title: `${ActivationStrings.activatingEnvironment}: ${environment.environmentPath.fsPath}`,
},
async () => {
await waitForShellIntegration(terminal);
Expand All @@ -225,9 +234,9 @@ export class TerminalManagerImpl implements TerminalManager {
} else {
traceVerbose(`Environment ${environment.environmentPath.fsPath} is not activatable`);
}
} else if (actType === 'off') {
} else if (actType === ACT_TYPE_OFF) {
traceInfo(`"python-envs.terminal.autoActivationType" is set to "${actType}", skipping auto activation`);
} else if (actType === 'shellStartup') {
} else if (actType === ACT_TYPE_SHELL) {
traceInfo(
`"python-envs.terminal.autoActivationType" is set to "${actType}", terminal should be activated by shell startup script`,
);
Expand All @@ -237,7 +246,7 @@ export class TerminalManagerImpl implements TerminalManager {
public async create(environment: PythonEnvironment, options: PythonTerminalCreateOptions): Promise<Terminal> {
const autoActType = getAutoActivationType();
let envVars = options.env;
if (autoActType === 'shellStartup') {
if (autoActType === ACT_TYPE_SHELL) {
const vars = await Promise.all(this.startupEnvProviders.map((p) => p.getEnvVariables(environment)));

vars.forEach((varMap) => {
Expand All @@ -249,6 +258,7 @@ export class TerminalManagerImpl implements TerminalManager {
});
}

// Uncomment the code line below after the issue is resolved:
// https://github.com/microsoft/vscode-python-environments/issues/172
// const name = options.name ?? `Python: ${environment.displayName}`;
const newTerminal = createTerminal({
Expand All @@ -266,7 +276,7 @@ export class TerminalManagerImpl implements TerminalManager {
isTransient: options.isTransient,
});

if (autoActType === 'command') {
if (autoActType === ACT_TYPE_COMMAND) {
if (options.disableActivation) {
this.skipActivationOnOpen.add(newTerminal);
return newTerminal;
Expand Down Expand Up @@ -360,9 +370,9 @@ export class TerminalManagerImpl implements TerminalManager {

public async initialize(api: PythonEnvironmentApi): Promise<void> {
const actType = getAutoActivationType();
if (actType === 'command') {
if (actType === ACT_TYPE_COMMAND) {
await Promise.all(terminals().map(async (t) => this.activateUsingCommand(api, t)));
} else if (actType === 'shellStartup') {
} else if (actType === ACT_TYPE_SHELL) {
const shells = new Set(
terminals()
.map((t) => identifyTerminalShell(t))
Expand Down
3 changes: 3 additions & 0 deletions src/features/terminal/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,9 @@ export async function getEnvironmentForTerminal(
return env;
}

export const ACT_TYPE_SHELL = 'shellStartup';
export const ACT_TYPE_COMMAND = 'command';
export const ACT_TYPE_OFF = 'off';
export type AutoActivationType = 'off' | 'command' | 'shellStartup';
export function getAutoActivationType(): AutoActivationType {
// 'startup' auto-activation means terminal is activated via shell startup scripts.
Expand Down