Skip to content

Commit a4a586d

Browse files
committed
add some clarifications
1 parent 9caba3f commit a4a586d

4 files changed

Lines changed: 178 additions & 19 deletions

File tree

src/managers/builtin/pipUtils.ts

Lines changed: 36 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -78,17 +78,19 @@ async function getCommonPackages(): Promise<Installable[]> {
7878
}
7979

8080
async function selectWorkspaceOrCommon(
81-
installable: Installable[],
81+
api: PythonEnvironmentApi,
82+
projects: PythonProject[] | undefined,
83+
hasWorkspaceDeps: boolean,
8284
common: Installable[],
8385
showSkipOption: boolean,
8486
installed: string[],
8587
): Promise<PipPackages | undefined> {
86-
if (installable.length === 0 && common.length === 0) {
88+
if (!hasWorkspaceDeps && common.length === 0) {
8789
return undefined;
8890
}
8991

9092
const items: QuickPickItem[] = [];
91-
if (installable.length > 0) {
93+
if (hasWorkspaceDeps) {
9294
items.push({
9395
label: PackageManagement.workspaceDependencies,
9496
description: PackageManagement.workspaceDependenciesDescription,
@@ -124,6 +126,12 @@ async function selectWorkspaceOrCommon(
124126
if (selected && !Array.isArray(selected)) {
125127
try {
126128
if (selected.label === PackageManagement.workspaceDependencies) {
129+
// Lazy load: only search for dependencies when user selects this option
130+
const installable = await getProjectInstallable(api, projects);
131+
if (installable.length === 0) {
132+
// No dependencies found, return to picker
133+
return selectWorkspaceOrCommon(api, projects, hasWorkspaceDeps, common, showSkipOption, installed);
134+
}
127135
return await selectFromInstallableToInstall(installable, undefined, { showBackButton });
128136
} else if (selected.label === PackageManagement.searchCommonPackages) {
129137
return await selectFromCommonPackagesToInstall(common, installed, undefined, { showBackButton });
@@ -136,7 +144,7 @@ async function selectWorkspaceOrCommon(
136144
// eslint-disable-next-line @typescript-eslint/no-explicit-any
137145
} catch (ex: any) {
138146
if (ex === QuickInputButtons.Back) {
139-
return selectWorkspaceOrCommon(installable, common, showSkipOption, installed);
147+
return selectWorkspaceOrCommon(api, projects, hasWorkspaceDeps, common, showSkipOption, installed);
140148
}
141149
}
142150
}
@@ -155,14 +163,36 @@ export async function getWorkspacePackagesToInstall(
155163
environment?: PythonEnvironment,
156164
log?: LogOutputChannel,
157165
): Promise<PipPackages | undefined> {
158-
const installable = (await getProjectInstallable(api, project)) ?? [];
166+
// Quick check if any dependency files exist (don't load/parse them yet)
167+
const hasWorkspaceDeps = await hasProjectDependencies(project);
159168
let common = await getCommonPackages();
160169
let installed: string[] | undefined;
161170
if (environment) {
162171
installed = (await refreshPipPackages(environment, log, { showProgress: true }))?.map((pkg) => pkg.name);
163172
common = mergePackages(common, installed ?? []);
164173
}
165-
return selectWorkspaceOrCommon(installable, common, !!options.showSkipOption, installed ?? []);
174+
return selectWorkspaceOrCommon(api, project, hasWorkspaceDeps, common, !!options.showSkipOption, installed ?? []);
175+
}
176+
177+
/**
178+
* Quickly check if any project dependency files exist.
179+
* This is a fast check that doesn't parse files, used to determine whether to show the option.
180+
*/
181+
export async function hasProjectDependencies(projects?: PythonProject[]): Promise<boolean> {
182+
if (!projects || projects.length === 0) {
183+
return false;
184+
}
185+
const exclude = '**/{.venv*,.git,.nox,.tox,.conda,site-packages,__pypackages__}/**';
186+
187+
// Quick check: just see if ANY files exist (maxResults=1 for performance)
188+
const results = await Promise.all([
189+
findFiles('**/*requirements*.txt', exclude, 1),
190+
findFiles('*requirements*.txt', exclude, 1),
191+
findFiles('**/requirements/*.txt', exclude, 1),
192+
findFiles('**/pyproject.toml', exclude, 1),
193+
]);
194+
195+
return results.some((uris) => uris.length > 0);
166196
}
167197

168198
export async function getProjectInstallable(

src/managers/builtin/venvStepBasedFlow.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,8 @@ async function enterEnvironmentName(state: VenvCreationState): Promise<StepFunct
203203

204204
// Handle cancellation (Escape key or dialog close)
205205
if (!name) {
206+
// Mark that the user cancelled, preventing fallback package selection
207+
state.packageSelectionCompleted = false;
206208
return null; // Exit the flow without creating an environment
207209
}
208210

src/test/features/creators/autoFindProjects.unit.test.ts

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,15 @@
1+
import assert from 'assert';
12
import * as path from 'path';
23
import * as sinon from 'sinon';
34
import * as typmoq from 'typemoq';
4-
import * as wapi from '../../../common/workspace.apis';
5-
import * as winapi from '../../../common/window.apis';
6-
import { PythonProjectManager } from '../../../internal.api';
7-
import { createDeferred } from '../../../common/utils/deferred';
8-
import { AutoFindProjects } from '../../../features/creators/autoFindProjects';
9-
import assert from 'assert';
105
import { Uri } from 'vscode';
116
import { PythonProject } from '../../../api';
127
import { sleep } from '../../../common/utils/asyncUtils';
8+
import { createDeferred } from '../../../common/utils/deferred';
9+
import * as winapi from '../../../common/window.apis';
10+
import * as wapi from '../../../common/workspace.apis';
11+
import { AutoFindProjects } from '../../../features/creators/autoFindProjects';
12+
import { PythonProjectManager } from '../../../internal.api';
1313

1414
suite('Auto Find Project tests', () => {
1515
let findFilesStub: sinon.SinonStub;

src/test/managers/builtin/pipUtils.unit.test.ts

Lines changed: 134 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import { CancellationToken, Progress, ProgressOptions, Uri } from 'vscode';
55
import { PythonEnvironmentApi, PythonProject } from '../../../api';
66
import * as winapi from '../../../common/window.apis';
77
import * as wapi from '../../../common/workspace.apis';
8-
import { getProjectInstallable } from '../../../managers/builtin/pipUtils';
8+
import { getProjectInstallable, hasProjectDependencies } from '../../../managers/builtin/pipUtils';
99

1010
suite('Pip Utils - getProjectInstallable', () => {
1111
let findFilesStub: sinon.SinonStub;
@@ -216,13 +216,15 @@ suite('Pip Utils - getProjectInstallable', () => {
216216
});
217217

218218
test('should handle cancellation during file search', async () => {
219-
// Arrange: Create a cancellation token that is already cancelled
219+
// ARRANGE: Simulate a scenario where the user cancels the operation
220+
// Step 1: Create a pre-cancelled token to simulate user clicking "Cancel" button
220221
const cancelledToken: CancellationToken = {
221222
isCancellationRequested: true,
222223
onCancellationRequested: () => ({ dispose: () => {} }),
223224
};
224225

225-
// Mock withProgress to immediately call the callback with the cancelled token
226+
// Step 2: Override withProgress stub to pass the cancelled token to the search callback
227+
// This simulates the progress dialog being cancelled and the token being propagated
226228
withProgressStub.callsFake(
227229
async (
228230
_options: ProgressOptions,
@@ -231,24 +233,149 @@ suite('Pip Utils - getProjectInstallable', () => {
231233
token: CancellationToken,
232234
) => Thenable<unknown>,
233235
) => {
236+
// Execute the callback with the cancelled token (simulating cancellation during the operation)
234237
return await callback({} as Progress<{ message?: string; increment?: number }>, cancelledToken);
235238
},
236239
);
237240

238-
// Mock findFiles to check that token is passed
241+
// Step 3: Mock findFiles to verify the cancelled token is properly passed through
242+
// This ensures cancellation propagates from withProgress -> getProjectInstallable -> findFiles
239243
findFilesStub.callsFake((_pattern: string, _exclude: string, _maxResults: number, token: CancellationToken) => {
240-
// Verify the cancellation token is passed to findFiles
244+
// VERIFY: The same cancellation token should be passed to each findFiles call
241245
assert.strictEqual(token, cancelledToken, 'Cancellation token should be passed to findFiles');
242246
return Promise.resolve([]);
243247
});
244248

245-
// Act: Call getProjectInstallable
249+
// ACT: Call the function under test
246250
const workspacePath = Uri.file('/test/path/root').fsPath;
247251
const projects = [{ name: 'workspace', uri: Uri.file(workspacePath) }];
248252
await getProjectInstallable(mockApi as PythonEnvironmentApi, projects);
249253

250-
// Assert: Verify findFiles was called with the cancellation token
254+
// ASSERT: Verify the cancellation token was passed to all file search operations
255+
// Even though cancelled, the function should attempt all searches (they'll just return empty quickly)
251256
assert.ok(findFilesStub.called, 'findFiles should be called');
257+
// getProjectInstallable searches for dependencies using 4 different file patterns
252258
assert.strictEqual(findFilesStub.callCount, 4, 'Should call findFiles 4 times for different patterns');
253259
});
254260
});
261+
262+
suite('Pip Utils - hasProjectDependencies', () => {
263+
let findFilesStub: sinon.SinonStub;
264+
265+
setup(() => {
266+
findFilesStub = sinon.stub(wapi, 'findFiles');
267+
});
268+
269+
teardown(() => {
270+
sinon.restore();
271+
});
272+
273+
test('should return true when requirements.txt exists', async () => {
274+
// Arrange: Mock findFiles to return a requirements file
275+
findFilesStub.callsFake((pattern: string, _exclude: string, maxResults?: number) => {
276+
// Verify maxResults=1 is used for performance (quick check)
277+
assert.strictEqual(maxResults, 1, 'Should use maxResults=1 for quick check');
278+
279+
if (pattern === '*requirements*.txt') {
280+
return Promise.resolve([Uri.file('/test/path/root/requirements.txt')]);
281+
}
282+
return Promise.resolve([]);
283+
});
284+
285+
// Act
286+
const projects = [{ name: 'workspace', uri: Uri.file('/test/path/root') }];
287+
const result = await hasProjectDependencies(projects);
288+
289+
// Assert
290+
assert.strictEqual(result, true, 'Should return true when requirements files exist');
291+
});
292+
293+
test('should return true when pyproject.toml exists', async () => {
294+
// Arrange: Mock findFiles to return pyproject.toml
295+
findFilesStub.callsFake((pattern: string, _exclude: string, maxResults?: number) => {
296+
assert.strictEqual(maxResults, 1, 'Should use maxResults=1 for quick check');
297+
298+
if (pattern === '**/pyproject.toml') {
299+
return Promise.resolve([Uri.file('/test/path/root/pyproject.toml')]);
300+
}
301+
return Promise.resolve([]);
302+
});
303+
304+
// Act
305+
const projects = [{ name: 'workspace', uri: Uri.file('/test/path/root') }];
306+
const result = await hasProjectDependencies(projects);
307+
308+
// Assert
309+
assert.strictEqual(result, true, 'Should return true when pyproject.toml exists');
310+
});
311+
312+
test('should return false when no dependency files exist', async () => {
313+
// Arrange: Mock findFiles to return empty arrays
314+
findFilesStub.resolves([]);
315+
316+
// Act
317+
const projects = [{ name: 'workspace', uri: Uri.file('/test/path/root') }];
318+
const result = await hasProjectDependencies(projects);
319+
320+
// Assert
321+
assert.strictEqual(result, false, 'Should return false when no dependency files exist');
322+
// Verify all 4 patterns were checked
323+
assert.strictEqual(findFilesStub.callCount, 4, 'Should check all 4 file patterns');
324+
});
325+
326+
test('should return false when no projects provided', async () => {
327+
// Act
328+
const result = await hasProjectDependencies(undefined);
329+
330+
// Assert
331+
assert.strictEqual(result, false, 'Should return false when no projects provided');
332+
assert.ok(!findFilesStub.called, 'Should not call findFiles when no projects');
333+
});
334+
335+
test('should return false when empty projects array provided', async () => {
336+
// Act
337+
const result = await hasProjectDependencies([]);
338+
339+
// Assert
340+
assert.strictEqual(result, false, 'Should return false when empty projects array');
341+
assert.ok(!findFilesStub.called, 'Should not call findFiles when projects array is empty');
342+
});
343+
344+
test('should use maxResults=1 for all patterns for performance', async () => {
345+
// Arrange: Track all maxResults values
346+
const maxResultsUsed: (number | undefined)[] = [];
347+
findFilesStub.callsFake((_pattern: string, _exclude: string, maxResults?: number) => {
348+
maxResultsUsed.push(maxResults);
349+
return Promise.resolve([]);
350+
});
351+
352+
// Act
353+
const projects = [{ name: 'workspace', uri: Uri.file('/test/path/root') }];
354+
await hasProjectDependencies(projects);
355+
356+
// Assert: All calls should use maxResults=1 for performance
357+
assert.strictEqual(maxResultsUsed.length, 4, 'Should make 4 findFiles calls');
358+
maxResultsUsed.forEach((value, index) => {
359+
assert.strictEqual(value, 1, `Call ${index + 1} should use maxResults=1`);
360+
});
361+
});
362+
363+
test('should short-circuit when first pattern finds a file', async () => {
364+
// Arrange: First pattern returns a result
365+
findFilesStub.callsFake((pattern: string) => {
366+
if (pattern === '**/*requirements*.txt') {
367+
return Promise.resolve([Uri.file('/test/path/root/dev-requirements.txt')]);
368+
}
369+
return Promise.resolve([]);
370+
});
371+
372+
// Act
373+
const projects = [{ name: 'workspace', uri: Uri.file('/test/path/root') }];
374+
const result = await hasProjectDependencies(projects);
375+
376+
// Assert: Should still return true even though only first pattern matched
377+
assert.strictEqual(result, true, 'Should return true when any pattern finds files');
378+
// Note: All 4 patterns are checked in parallel with Promise.all, so all 4 calls happen
379+
assert.strictEqual(findFilesStub.callCount, 4, 'All patterns checked in parallel');
380+
});
381+
});

0 commit comments

Comments
 (0)