Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 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
6 changes: 6 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,12 @@
"%python-envs.terminal.autoActivationType.off%"
],
"scope": "machine"
},
"python.terminal.useEnvFile": {
"type": "boolean",
"description": "%python-envs.terminal.useEnvFile.description%",
"default": false,
"scope": "resource"
}
}
},
Expand Down
3 changes: 2 additions & 1 deletion package.nls.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,5 +36,6 @@
"python-envs.terminal.deactivate.title": "Deactivate Environment in Current Terminal",
"python-envs.uninstallPackage.title": "Uninstall Package",
"python-envs.revealProjectInExplorer.title": "Reveal Project in Explorer",
"python-envs.runPetInTerminal.title": "Run Python Environment Tool (PET) in Terminal"
"python-envs.runPetInTerminal.title": "Run Python Environment Tool (PET) in Terminal",
"python-envs.terminal.useEnvFile.description": "Controls whether environment variables from .env files and python.envFile setting are injected into terminals"
}
182 changes: 158 additions & 24 deletions src/features/terminal/terminalEnvVarInjector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import * as fse from 'fs-extra';
import * as path from 'path';
import {
ConfigurationChangeEvent,
Disposable,
EnvironmentVariableScope,
GlobalEnvironmentVariableCollection,
Expand All @@ -12,7 +13,8 @@ import {
} from 'vscode';
import { traceError, traceVerbose } from '../../common/logging';
import { resolveVariables } from '../../common/utils/internalVariables';
import { getConfiguration, getWorkspaceFolder } from '../../common/workspace.apis';
import { getConfiguration, getWorkspaceFolder, onDidChangeConfiguration } from '../../common/workspace.apis';
import { showInformationMessage } from '../../common/window.apis';
import { EnvVarManager } from '../execution/envVariableManager';

/**
Expand All @@ -21,6 +23,7 @@ import { EnvVarManager } from '../execution/envVariableManager';
*/
export class TerminalEnvVarInjector implements Disposable {
private disposables: Disposable[] = [];
private readonly previousSettingsState = new Map<string, { useEnvFile: boolean | undefined; envFile: string | undefined }>();

constructor(
private readonly envVarCollection: GlobalEnvironmentVariableCollection,
Expand All @@ -35,6 +38,21 @@ export class TerminalEnvVarInjector implements Disposable {
private async initialize(): Promise<void> {
traceVerbose('TerminalEnvVarInjector: Initializing environment variable injection');

// Initialize previous settings state for all workspaces
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@copilot
no this is now too complicated. lets simplify it to just be:

if user updates python.envFile and they don't have python.useEnvFile set to true, show a notification.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Simplified the notification logic as requested. Now only shows a notification when user updates python.envFile and they don't have python.useEnvFile set to true. Removed the complex logic that tracked multiple settings and scenarios. Commit: 8ac5db2

const workspaceFolders = workspace.workspaceFolders;
if (workspaceFolders) {
for (const folder of workspaceFolders) {
this.updatePreviousSettingsState(folder);
}
}

// Listen for configuration changes to show notifications when settings change
this.disposables.push(
onDidChangeConfiguration((event: ConfigurationChangeEvent) => {
this.handleConfigurationChange(event);
}),
);

// Listen for environment variable changes from the manager
this.disposables.push(
this.envVarManager.onDidChangeEnvironmentVariables((args) => {
Expand Down Expand Up @@ -70,6 +88,109 @@ export class TerminalEnvVarInjector implements Disposable {
await this.updateEnvironmentVariables();
}

/**
* Handle configuration changes and show notifications when relevant settings change.
*/
private handleConfigurationChange(event: ConfigurationChangeEvent): void {
const workspaceFolders = workspace.workspaceFolders;
if (!workspaceFolders) {
return;
}

for (const folder of workspaceFolders) {
if (event.affectsConfiguration('python.terminal.useEnvFile', folder.uri) ||
event.affectsConfiguration('python.envFile', folder.uri)) {

const folderKey = folder.uri.toString();
const previousState = this.previousSettingsState.get(folderKey);
const currentState = this.getCurrentSettingsState(folder);

if (previousState && this.shouldShowNotification(previousState, currentState)) {
this.showSettingsChangeNotification(currentState);
}

this.previousSettingsState.set(folderKey, currentState);

// Update environment variables for this workspace
this.updateEnvironmentVariables(folder).catch((error) => {
traceError('Failed to update environment variables after configuration change:', error);
});
}
}
}

/**
* Get current settings state for a workspace.
*/
private getCurrentSettingsState(workspaceFolder: WorkspaceFolder): { useEnvFile: boolean | undefined; envFile: string | undefined } {
const config = getConfiguration('python', workspaceFolder.uri);
return {
useEnvFile: config.get<boolean>('terminal.useEnvFile'),
envFile: config.get<string>('envFile')
};
}

/**
* Update the previous settings state for a workspace.
*/
private updatePreviousSettingsState(workspaceFolder: WorkspaceFolder): void {
const folderKey = workspaceFolder.uri.toString();
this.previousSettingsState.set(folderKey, this.getCurrentSettingsState(workspaceFolder));
}

/**
* Determine if we should show a notification based on settings changes.
*/
private shouldShowNotification(
previousState: { useEnvFile: boolean | undefined; envFile: string | undefined },
currentState: { useEnvFile: boolean | undefined; envFile: string | undefined }
): boolean {
// Show notification if:
// 1. useEnvFile changed from undefined/false to true
// 2. envFile changed from undefined to set (any string value)
// And ensure both settings are configured correctly

const useEnvFileChanged = !previousState.useEnvFile && currentState.useEnvFile;
const envFileChanged = !previousState.envFile && !!currentState.envFile;

if (useEnvFileChanged || envFileChanged) {
// If one is set but the other isn't, show notification
if ((currentState.envFile && !currentState.useEnvFile) ||
(currentState.useEnvFile && !currentState.envFile)) {
return true;
}
}

return false;
}

/**
* Show notification about settings configuration.
*/
private showSettingsChangeNotification(currentState: { useEnvFile: boolean | undefined; envFile: string | undefined }): void {
let message: string;

if (currentState.envFile && !currentState.useEnvFile) {
message = 'The python.envFile setting is configured but will not take effect in terminals. Enable the "python.terminal.useEnvFile" setting to use environment variables from .env files in terminals.';
} else if (currentState.useEnvFile && !currentState.envFile) {
message = 'The python.terminal.useEnvFile setting is enabled. Consider setting "python.envFile" to specify a custom .env file path, or ensure a .env file exists in your workspace root.';
} else {
return; // Both are properly configured, no notification needed
}

showInformationMessage(message, 'Open Settings').then((selection) => {
if (selection === 'Open Settings') {
// Open VS Code settings to the relevant setting
import('vscode').then(vscode => {
const settingToOpen = currentState.envFile && !currentState.useEnvFile
? 'python.terminal.useEnvFile'
: 'python.envFile';
vscode.commands.executeCommand('workbench.action.openSettings', settingToOpen);
});
}
});
}

/**
* Update environment variables in the terminal collection.
*/
Expand Down Expand Up @@ -109,37 +230,50 @@ export class TerminalEnvVarInjector implements Disposable {
private async injectEnvironmentVariablesForWorkspace(workspaceFolder: WorkspaceFolder): Promise<void> {
const workspaceUri = workspaceFolder.uri;
try {
const envVars = await this.envVarManager.getEnvironmentVariables(workspaceUri);

// Check if environment variable injection is enabled
const config = getConfiguration('python', workspaceUri);
const useEnvFile = config.get<boolean>('terminal.useEnvFile', false);

// use scoped environment variable collection
const envVarScope = this.getEnvironmentVariableCollectionScoped({ workspaceFolder });
envVarScope.clear(); // Clear existing variables for this workspace

// Track which .env file is being used for logging
const config = getConfiguration('python', workspaceUri);
const envFilePath = config.get<string>('envFile');
const resolvedEnvFilePath: string | undefined = envFilePath
? path.resolve(resolveVariables(envFilePath, workspaceUri))
: undefined;
const defaultEnvFilePath: string = path.join(workspaceUri.fsPath, '.env');

let activeEnvFilePath: string = resolvedEnvFilePath || defaultEnvFilePath;
if (activeEnvFilePath && (await fse.pathExists(activeEnvFilePath))) {
traceVerbose(`TerminalEnvVarInjector: Using env file: ${activeEnvFilePath}`);
} else {
// Only inject if useEnvFile is true
if (useEnvFile) {
traceVerbose(
`TerminalEnvVarInjector: No .env file found for workspace: ${workspaceUri.fsPath}, not injecting environment variables.`,
`TerminalEnvVarInjector: Environment variable injection enabled for workspace: ${workspaceUri.fsPath}`,
);
return; // No .env file to inject
}

for (const [key, value] of Object.entries(envVars)) {
if (value === undefined) {
// Remove the environment variable if the value is undefined
envVarScope.delete(key);
const envVars = await this.envVarManager.getEnvironmentVariables(workspaceUri);

// Track which .env file is being used for logging
const envFilePath = config.get<string>('envFile');
const resolvedEnvFilePath: string | undefined = envFilePath
? path.resolve(resolveVariables(envFilePath, workspaceUri))
: undefined;
const defaultEnvFilePath: string = path.join(workspaceUri.fsPath, '.env');

let activeEnvFilePath: string = resolvedEnvFilePath || defaultEnvFilePath;
if (activeEnvFilePath && (await fse.pathExists(activeEnvFilePath))) {
traceVerbose(`TerminalEnvVarInjector: Using env file: ${activeEnvFilePath}`);

for (const [key, value] of Object.entries(envVars)) {
if (value === undefined) {
// Remove the environment variable if the value is undefined
envVarScope.delete(key);
} else {
envVarScope.replace(key, value);
}
}
} else {
envVarScope.replace(key, value);
traceVerbose(
`TerminalEnvVarInjector: No .env file found for workspace: ${workspaceUri.fsPath}, not injecting environment variables.`,
);
}
} else {
traceVerbose(
`TerminalEnvVarInjector: Environment variable injection disabled for workspace: ${workspaceUri.fsPath}`,
);
}
} catch (error) {
traceError(
Expand All @@ -154,7 +288,7 @@ export class TerminalEnvVarInjector implements Disposable {
*/
dispose(): void {
traceVerbose('TerminalEnvVarInjector: Disposing');
this.disposables.forEach((disposable) => disposable.dispose());
this.disposables.forEach((disposable) => disposable?.dispose());
this.disposables = [];

// Clear all environment variables from the collection
Expand Down
12 changes: 10 additions & 2 deletions src/test/features/terminalEnvVarInjectorBasic.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@

import * as sinon from 'sinon';
import * as typeMoq from 'typemoq';
import { GlobalEnvironmentVariableCollection, workspace } from 'vscode';
import { GlobalEnvironmentVariableCollection, workspace, EnvironmentVariableCollection } from 'vscode';
import { EnvVarManager } from '../../features/execution/envVariableManager';
import { TerminalEnvVarInjector } from '../../features/terminal/terminalEnvVarInjector';
import * as workspaceApi from '../../common/workspace.apis';

interface MockScopedCollection {
clear: sinon.SinonStub;
Expand All @@ -20,6 +21,7 @@ suite('TerminalEnvVarInjector Basic Tests', () => {
let mockScopedCollection: MockScopedCollection;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
let workspaceFoldersStub: any;
let onDidChangeConfigurationStub: sinon.SinonStub;

setup(() => {
envVarCollection = typeMoq.Mock.ofType<GlobalEnvironmentVariableCollection>();
Expand All @@ -40,9 +42,15 @@ suite('TerminalEnvVarInjector Basic Tests', () => {
};

// Setup environment variable collection to return scoped collection
envVarCollection.setup((x) => x.getScoped(typeMoq.It.isAny())).returns(() => mockScopedCollection as any);
envVarCollection.setup((x) => x.getScoped(typeMoq.It.isAny())).returns(() => mockScopedCollection as unknown as EnvironmentVariableCollection);
envVarCollection.setup((x) => x.clear()).returns(() => {});

// Mock onDidChangeConfiguration to return a disposable
onDidChangeConfigurationStub = sinon.stub(workspaceApi, 'onDidChangeConfiguration');
onDidChangeConfigurationStub.returns({
dispose: () => {}
});

// Setup minimal mocks for event subscriptions
envVarManager
.setup((m) => m.onDidChangeEnvironmentVariables)
Expand Down