Skip to content

Commit d2f08e2

Browse files
eleanorjboydeleanorjboyd
committed
address comments
1 parent 0a3ab62 commit d2f08e2

2 files changed

Lines changed: 83 additions & 67 deletions

File tree

‎src/features/settings/settingHelpers.ts‎

Lines changed: 48 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import * as path from 'path';
22
import { ConfigurationScope, ConfigurationTarget, Uri, WorkspaceConfiguration, WorkspaceFolder } from 'vscode';
33
import { PythonProject } from '../../api';
44
import { DEFAULT_ENV_MANAGER_ID, DEFAULT_PACKAGE_MANAGER_ID } from '../../common/constants';
5-
import { traceError, traceInfo, traceWarn } from '../../common/logging';
5+
import { traceError, traceInfo, traceVerbose, traceWarn } from '../../common/logging';
66
import * as workspaceApis from '../../common/workspace.apis';
77
import { PythonProjectManager, PythonProjectSettings } from '../../internal.api';
88

@@ -81,8 +81,16 @@ export function getDefaultPkgManagerSetting(
8181
return defaultManager;
8282
}
8383

84+
function traceIgnoredGlobalManagerEdits(functionName: string, count: number): void {
85+
if (count > 0) {
86+
traceVerbose(
87+
`[${functionName}] Ignoring ${count} edit(s) without a project because python-envs does not persist manager defaults to User/global settings.`,
88+
);
89+
}
90+
}
91+
8492
export interface EditAllManagerSettings {
85-
// undefined means global
93+
// Edits without a project are ignored; python-envs does not persist manager defaults to User/global settings.
8694
project?: PythonProject;
8795
envManager: string;
8896
packageManager: string;
@@ -95,24 +103,25 @@ interface EditAllManagerSettingsInternal {
95103
export async function setAllManagerSettings(edits: EditAllManagerSettings[]): Promise<void> {
96104
const noWorkspace: EditAllManagerSettingsInternal[] = [];
97105
const workspaces = new Map<WorkspaceFolder, EditAllManagerSettingsInternal[]>();
98-
edits
99-
.filter((e) => !!e.project)
100-
.map((e) => e as EditAllManagerSettingsInternal)
101-
.forEach((e) => {
102-
const w = workspaceApis.getWorkspaceFolder(e.project.uri);
103-
if (w) {
104-
workspaces.set(w, [
105-
...(workspaces.get(w) || []),
106-
{ project: e.project, envManager: e.envManager, packageManager: e.packageManager },
107-
]);
108-
} else {
109-
noWorkspace.push({ project: e.project, envManager: e.envManager, packageManager: e.packageManager });
110-
}
111-
});
106+
const projectEdits = edits.filter((e): e is EditAllManagerSettingsInternal => !!e.project);
107+
traceIgnoredGlobalManagerEdits('setAllManagerSettings', edits.length - projectEdits.length);
108+
projectEdits.forEach((e) => {
109+
const w = workspaceApis.getWorkspaceFolder(e.project.uri);
110+
if (w) {
111+
workspaces.set(w, [
112+
...(workspaces.get(w) || []),
113+
{ project: e.project, envManager: e.envManager, packageManager: e.packageManager },
114+
]);
115+
} else {
116+
noWorkspace.push({ project: e.project, envManager: e.envManager, packageManager: e.packageManager });
117+
}
118+
});
112119

113120
noWorkspace.forEach((e) => {
114121
if (e.project) {
115-
traceInfo(`Unable to find workspace for ${e.project.uri.fsPath}, will use global settings for this.`);
122+
traceInfo(
123+
`Unable to find workspace for ${e.project.uri.fsPath}, skipping settings update because no User/global fallback is written.`,
124+
);
116125
}
117126
});
118127

@@ -199,7 +208,7 @@ export async function setAllManagerSettings(edits: EditAllManagerSettings[]): Pr
199208
}
200209

201210
export interface EditEnvManagerSettings {
202-
// undefined means global
211+
// Edits without a project are ignored; python-envs does not persist manager defaults to User/global settings.
203212
project?: PythonProject;
204213
envManager: string;
205214
}
@@ -210,17 +219,16 @@ interface EditEnvManagerSettingsInternal {
210219
export async function setEnvironmentManager(edits: EditEnvManagerSettings[]): Promise<void> {
211220
const noWorkspace: EditEnvManagerSettingsInternal[] = [];
212221
const workspaces = new Map<WorkspaceFolder, EditEnvManagerSettingsInternal[]>();
213-
edits
214-
.filter((e) => !!e.project)
215-
.map((e) => e as EditEnvManagerSettingsInternal)
216-
.forEach((e) => {
217-
const w = workspaceApis.getWorkspaceFolder(e.project.uri);
218-
if (w) {
219-
workspaces.set(w, [...(workspaces.get(w) || []), { project: e.project, envManager: e.envManager }]);
220-
} else {
221-
noWorkspace.push({ project: e.project, envManager: e.envManager });
222-
}
223-
});
222+
const projectEdits = edits.filter((e): e is EditEnvManagerSettingsInternal => !!e.project);
223+
traceIgnoredGlobalManagerEdits('setEnvironmentManager', edits.length - projectEdits.length);
224+
projectEdits.forEach((e) => {
225+
const w = workspaceApis.getWorkspaceFolder(e.project.uri);
226+
if (w) {
227+
workspaces.set(w, [...(workspaces.get(w) || []), { project: e.project, envManager: e.envManager }]);
228+
} else {
229+
noWorkspace.push({ project: e.project, envManager: e.envManager });
230+
}
231+
});
224232

225233
noWorkspace.forEach((e) => {
226234
if (e.project) {
@@ -261,7 +269,7 @@ export async function setEnvironmentManager(edits: EditEnvManagerSettings[]): Pr
261269
}
262270

263271
export interface EditPackageManagerSettings {
264-
// undefined means global
272+
// Edits without a project are ignored; python-envs does not persist manager defaults to User/global settings.
265273
project?: PythonProject;
266274
packageManager: string;
267275
}
@@ -272,20 +280,16 @@ interface EditPackageManagerSettingsInternal {
272280
export async function setPackageManager(edits: EditPackageManagerSettings[]): Promise<void> {
273281
const noWorkspace: EditPackageManagerSettingsInternal[] = [];
274282
const workspaces = new Map<WorkspaceFolder, EditPackageManagerSettingsInternal[]>();
275-
edits
276-
.filter((e) => !!e.project)
277-
.map((e) => e as EditPackageManagerSettingsInternal)
278-
.forEach((e) => {
279-
const w = workspaceApis.getWorkspaceFolder(e.project.uri);
280-
if (w) {
281-
workspaces.set(w, [
282-
...(workspaces.get(w) || []),
283-
{ project: e.project, packageManager: e.packageManager },
284-
]);
285-
} else {
286-
noWorkspace.push({ project: e.project, packageManager: e.packageManager });
287-
}
288-
});
283+
const projectEdits = edits.filter((e): e is EditPackageManagerSettingsInternal => !!e.project);
284+
traceIgnoredGlobalManagerEdits('setPackageManager', edits.length - projectEdits.length);
285+
projectEdits.forEach((e) => {
286+
const w = workspaceApis.getWorkspaceFolder(e.project.uri);
287+
if (w) {
288+
workspaces.set(w, [...(workspaces.get(w) || []), { project: e.project, packageManager: e.packageManager }]);
289+
} else {
290+
noWorkspace.push({ project: e.project, packageManager: e.packageManager });
291+
}
292+
});
289293

290294
noWorkspace.forEach((e) => {
291295
if (e.project) {

‎src/test/features/settings/settingHelpers.unit.test.ts‎

Lines changed: 35 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import * as assert from 'assert';
33
import * as path from 'path';
44
import * as sinon from 'sinon';
55
import { ConfigurationTarget, Uri, WorkspaceFolder } from 'vscode';
6+
import * as logging from '../../../common/logging';
67
import * as workspaceApis from '../../../common/workspace.apis';
78
import {
89
addPythonProjectSetting,
@@ -22,20 +23,16 @@ function getTestWorkspacePath(): string {
2223
}
2324

2425
/**
25-
* These tests verify that settings ARE written when the value changes,
26-
* regardless of whether it's the default/system manager or not.
27-
*
28-
* Note: These tests focus on the global settings path (project=undefined) because
29-
* workspace-scoped tests would require mocking workspace.getWorkspaceFolder which
30-
* cannot be easily stubbed in unit tests.
26+
* These tests verify that manager edits without a project do not write settings
27+
* and are logged explicitly as ignored global edits.
3128
*/
3229
suite('Setting Helpers - Settings Write Behavior', () => {
3330
const SYSTEM_MANAGER_ID = 'ms-python.python:system';
3431
const VENV_MANAGER_ID = 'ms-python.python:venv';
3532
const PIP_MANAGER_ID = 'ms-python.python:pip';
3633
const CONDA_MANAGER_ID = 'ms-python.python:conda';
3734

38-
let updateCalls: Array<{ key: string; value: unknown; target: ConfigurationTarget }>;
35+
let updateCalls: Array<{ key: string; value: unknown; target: boolean | ConfigurationTarget | undefined }>;
3936

4037
setup(() => {
4138
updateCalls = [];
@@ -46,7 +43,7 @@ suite('Setting Helpers - Settings Write Behavior', () => {
4643
});
4744

4845
/**
49-
* Creates a mock WorkspaceConfiguration that tracks update calls
46+
* Creates a mock WorkspaceConfiguration that tracks update calls.
5047
*/
5148
function createMockConfig(options: {
5249
defaultEnvManagerGlobalValue?: string;
@@ -99,7 +96,7 @@ suite('Setting Helpers - Settings Write Behavior', () => {
9996
updateCalls.push({
10097
key: section,
10198
value,
102-
target: configurationTarget as ConfigurationTarget,
99+
target: configurationTarget,
103100
});
104101
return Promise.resolve();
105102
};
@@ -113,6 +110,7 @@ suite('Setting Helpers - Settings Write Behavior', () => {
113110
currentEnvManager: VENV_MANAGER_ID,
114111
});
115112
sinon.stub(workspaceApis, 'getConfiguration').returns(mockConfig);
113+
const traceVerboseStub = sinon.stub(logging, 'traceVerbose');
116114

117115
await setAllManagerSettings([
118116
{
@@ -122,10 +120,12 @@ suite('Setting Helpers - Settings Write Behavior', () => {
122120
},
123121
]);
124122

125-
const envManagerUpdates = updateCalls.filter(
126-
(c) => c.key === 'defaultEnvManager' && c.target === ConfigurationTarget.Global,
123+
const envManagerUpdates = updateCalls.filter((c) => c.key === 'defaultEnvManager');
124+
assert.strictEqual(envManagerUpdates.length, 0, 'Should never write defaultEnvManager for global edits');
125+
sinon.assert.calledWithMatch(
126+
traceVerboseStub,
127+
'[setAllManagerSettings] Ignoring 1 edit(s) without a project because python-envs does not persist manager defaults to User/global settings.',
127128
);
128-
assert.strictEqual(envManagerUpdates.length, 0, 'Should never write to user/global settings');
129129
});
130130

131131
test('should NOT write global defaultPackageManager even when value differs from current', async () => {
@@ -143,10 +143,12 @@ suite('Setting Helpers - Settings Write Behavior', () => {
143143
},
144144
]);
145145

146-
const pkgManagerUpdates = updateCalls.filter(
147-
(c) => c.key === 'defaultPackageManager' && c.target === ConfigurationTarget.Global,
146+
const pkgManagerUpdates = updateCalls.filter((c) => c.key === 'defaultPackageManager');
147+
assert.strictEqual(
148+
pkgManagerUpdates.length,
149+
0,
150+
'Should never write defaultPackageManager for global edits',
148151
);
149-
assert.strictEqual(pkgManagerUpdates.length, 0, 'Should never write to user/global settings');
150152
});
151153
});
152154

@@ -156,6 +158,7 @@ suite('Setting Helpers - Settings Write Behavior', () => {
156158
currentEnvManager: VENV_MANAGER_ID,
157159
});
158160
sinon.stub(workspaceApis, 'getConfiguration').returns(mockConfig);
161+
const traceVerboseStub = sinon.stub(logging, 'traceVerbose');
159162

160163
await setEnvironmentManager([
161164
{
@@ -164,10 +167,12 @@ suite('Setting Helpers - Settings Write Behavior', () => {
164167
},
165168
]);
166169

167-
const envManagerUpdates = updateCalls.filter(
168-
(c) => c.key === 'defaultEnvManager' && c.target === ConfigurationTarget.Global,
170+
const envManagerUpdates = updateCalls.filter((c) => c.key === 'defaultEnvManager');
171+
assert.strictEqual(envManagerUpdates.length, 0, 'Should never write defaultEnvManager for global edits');
172+
sinon.assert.calledWithMatch(
173+
traceVerboseStub,
174+
'[setEnvironmentManager] Ignoring 1 edit(s) without a project because python-envs does not persist manager defaults to User/global settings.',
169175
);
170-
assert.strictEqual(envManagerUpdates.length, 0, 'Should never write to user/global settings');
171176
});
172177
});
173178

@@ -177,6 +182,7 @@ suite('Setting Helpers - Settings Write Behavior', () => {
177182
currentPkgManager: PIP_MANAGER_ID,
178183
});
179184
sinon.stub(workspaceApis, 'getConfiguration').returns(mockConfig);
185+
const traceVerboseStub = sinon.stub(logging, 'traceVerbose');
180186

181187
await setPackageManager([
182188
{
@@ -185,10 +191,16 @@ suite('Setting Helpers - Settings Write Behavior', () => {
185191
},
186192
]);
187193

188-
const pkgManagerUpdates = updateCalls.filter(
189-
(c) => c.key === 'defaultPackageManager' && c.target === ConfigurationTarget.Global,
194+
const pkgManagerUpdates = updateCalls.filter((c) => c.key === 'defaultPackageManager');
195+
assert.strictEqual(
196+
pkgManagerUpdates.length,
197+
0,
198+
'Should never write defaultPackageManager for global edits',
199+
);
200+
sinon.assert.calledWithMatch(
201+
traceVerboseStub,
202+
'[setPackageManager] Ignoring 1 edit(s) without a project because python-envs does not persist manager defaults to User/global settings.',
190203
);
191-
assert.strictEqual(pkgManagerUpdates.length, 0, 'Should never write to user/global settings');
192204
});
193205
});
194206
});
@@ -210,7 +222,7 @@ suite('Setting Helpers - Empty Path Bug Fix', () => {
210222
index: 0,
211223
};
212224

213-
let updateCalls: Array<{ key: string; value: unknown; target: ConfigurationTarget }>;
225+
let updateCalls: Array<{ key: string; value: unknown; target: boolean | ConfigurationTarget | undefined }>;
214226

215227
setup(() => {
216228
updateCalls = [];
@@ -248,7 +260,7 @@ suite('Setting Helpers - Empty Path Bug Fix', () => {
248260
updateCalls.push({
249261
key: section,
250262
value,
251-
target: configurationTarget as ConfigurationTarget,
263+
target: configurationTarget,
252264
});
253265
return Promise.resolve();
254266
};

0 commit comments

Comments
 (0)