Skip to content

Commit 57f4248

Browse files
StellaHuang95StellaHuang95
authored
Some refactoring around inline script manager (#1685)
This PR addresses the following feedback: 1. Make error message handling a helper function. 2. Move all inline script related files into its own subfolders. No code logics or implementations are changed, it's pure refactoring to address previous feedback.
1 parent c0f89be commit 57f4248

16 files changed

Lines changed: 88 additions & 87 deletions

File tree

‎src/common/errors/utils.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,10 @@ import { commands, LogOutputChannel } from 'vscode';
33
import { Common } from '../localize';
44
import { showErrorMessage, showWarningMessage } from '../window.apis';
55

6+
export function getErrorMessage(error: unknown): string {
7+
return error instanceof Error ? error.message : String(error);
8+
}
9+
610
export function parseStack(ex: Error) {
711
if (ex.stack && Array.isArray(ex.stack)) {
812
const concatenated = { ...ex, stack: ex.stack.join('\n') };

src/common/inlineScriptCacheKey.ts renamed to src/common/inlineScript/cacheKey.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,8 @@
22
// Licensed under the MIT License.
33

44
import { createHash } from 'crypto';
5-
import { normalizePackageName } from '../managers/builtin/utils';
6-
import { normalizePath } from './utils/pathUtils';
5+
import { normalizePackageName } from '../../managers/builtin/utils';
6+
import { normalizePath } from '../utils/pathUtils';
77

88
/** Length, in hex chars, of the cache key returned by {@link computeCacheKey}. 16 = 64 bits of SHA-256; fixed-length and filesystem-safe. */
99
export const CACHE_KEY_HEX_LENGTH = 16;

src/common/inlineScriptCacheLayout.ts renamed to src/common/inlineScript/cacheLayout.ts

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,13 @@ import * as crypto from 'crypto';
55
import * as fsapi from 'fs-extra';
66
import * as path from 'path';
77
import { Uri } from 'vscode';
8-
import type { PythonEnvironment } from '../api';
9-
import { INLINE_SCRIPT_MANAGER_ID } from './constants';
10-
import { traceWarn } from './logging';
11-
import { isFileNotFoundError } from './utils/filesystem';
12-
import { normalizePath } from './utils/pathUtils';
13-
import { isWindows } from './utils/platformUtils';
14-
import { getVenvPythonPath } from './utils/virtualEnvironment';
8+
import type { PythonEnvironment } from '../../api';
9+
import { INLINE_SCRIPT_MANAGER_ID } from '../constants';
10+
import { traceWarn } from '../logging';
11+
import { isFileNotFoundError } from '../utils/filesystem';
12+
import { normalizePath } from '../utils/pathUtils';
13+
import { isWindows } from '../utils/platformUtils';
14+
import { getVenvPythonPath } from '../utils/virtualEnvironment';
1515

1616
/** Bump this and {@link META_SCHEMA_VERSION} together for incompatible cache formats. */
1717
export const INLINE_SCRIPT_CACHE_DIR_NAME = 'script-envs-v1';

src/common/inlineScriptInterpreter.ts renamed to src/common/inlineScript/interpreter.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
// Copyright (c) Microsoft Corporation. All rights reserved.
22
// Licensed under the MIT License.
33

4-
import { PythonEnvironment } from '../api';
5-
import { matchesPythonVersion } from './inlineScriptMetadata';
6-
import { traceWarn } from './logging';
7-
import { compareReleaseSegments, parseReleaseSegments } from './utils/pep440Release';
4+
import { PythonEnvironment } from '../../api';
5+
import { traceWarn } from '../logging';
6+
import { compareReleaseSegments, parseReleaseSegments } from '../utils/pep440Release';
7+
import { matchesPythonVersion } from './metadata';
88

99
/**
1010
* Pick the newest installed Python that can serve as a base interpreter for

src/common/inlineScriptMetadata.ts renamed to src/common/inlineScript/metadata.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,8 @@
44
import * as tomljs from '@iarna/toml';
55
import * as fs from 'fs/promises';
66
import { Uri } from 'vscode';
7-
import { traceVerbose, traceWarn } from './logging';
8-
import { compareReleaseSegments, parseReleaseSegments } from './utils/pep440Release';
7+
import { traceVerbose, traceWarn } from '../logging';
8+
import { compareReleaseSegments, parseReleaseSegments } from '../utils/pep440Release';
99

1010
/**
1111
* Parsed and validated PEP 723 `script` metadata block.

‎src/extension.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ import {
6565
} from './features/envCommands';
6666
import { PythonEnvironmentManagers } from './features/envManagers';
6767
import { EnvVarManager, PythonEnvVariableManager } from './features/execution/envVariableManager';
68-
import { InlineScriptLazyDetector } from './features/inlineScriptLazyDetector';
68+
import { InlineScriptLazyDetector } from './features/inlineScript/lazyDetector';
6969
import {
7070
applyInitialEnvironmentSelection,
7171
registerInterpreterSettingsChangeListener,
@@ -98,7 +98,7 @@ import { ProjectItem, PythonEnvTreeItem } from './features/views/treeViewItems';
9898
import { collectEnvironmentInfo, getEnvManagerAndPackageManagerConfigLevels, runPetInTerminalImpl } from './helpers';
9999
import { EnvironmentManagers, ProjectCreators, PythonProjectManager } from './internal.api';
100100
import { registerSystemPythonFeatures } from './managers/builtin/main';
101-
import { registerInlineScriptFeatures } from './managers/builtin/inlineScriptMain';
101+
import { registerInlineScriptFeatures } from './managers/builtin/inlineScript/main';
102102
import { SysPythonManager } from './managers/builtin/sysPythonManager';
103103
import {
104104
createNativePythonFinder,

src/features/inlineScriptLazyDetector.ts renamed to src/features/inlineScript/lazyDetector.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,17 +3,17 @@
33

44
import * as path from 'path';
55
import { Disposable, TextDocument, TextDocumentChangeEvent, Uri } from 'vscode';
6-
import { readInlineScriptMetadataFromFile } from '../common/inlineScriptMetadata';
7-
import { traceVerbose, traceWarn } from '../common/logging';
8-
import { EventNames } from '../common/telemetry/constants';
9-
import { sendTelemetryEvent } from '../common/telemetry/sender';
6+
import { readInlineScriptMetadataFromFile } from '../../common/inlineScript/metadata';
7+
import { traceVerbose, traceWarn } from '../../common/logging';
8+
import { EventNames } from '../../common/telemetry/constants';
9+
import { sendTelemetryEvent } from '../../common/telemetry/sender';
1010
import {
1111
getOpenTextDocuments,
1212
getWorkspaceFolder,
1313
onDidChangeTextDocument,
1414
onDidOpenTextDocument,
1515
onDidSaveTextDocument,
16-
} from '../common/workspace.apis';
16+
} from '../../common/workspace.apis';
1717

1818
/**
1919
* Silent on-open / on-save detector for `.py` files that declare

src/managers/builtin/inlineScriptEnvManager.ts renamed to src/managers/builtin/inlineScript/envManager.ts

Lines changed: 24 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,9 @@ import {
1818
RefreshEnvironmentsScope,
1919
ResolveEnvironmentContext,
2020
SetEnvironmentScope,
21-
} from '../../api';
22-
import { computeCacheKey } from '../../common/inlineScriptCacheKey';
21+
} from '../../../api';
22+
import { getErrorMessage } from '../../../common/errors/utils';
23+
import { computeCacheKey } from '../../../common/inlineScript/cacheKey';
2324
import {
2425
META_SCHEMA_VERSION,
2526
getBaseInterpreterStatus,
@@ -29,21 +30,21 @@ import {
2930
inspectMetaJson,
3031
resolveCacheEntryPath,
3132
writeMetaJson,
32-
} from '../../common/inlineScriptCacheLayout';
33-
import { pickCompatibleInterpreter } from '../../common/inlineScriptInterpreter';
33+
} from '../../../common/inlineScript/cacheLayout';
34+
import { pickCompatibleInterpreter } from '../../../common/inlineScript/interpreter';
3435
import {
3536
InlineScriptMetadata,
3637
matchesPythonVersion,
3738
readInlineScriptMetadataFromFile,
38-
} from '../../common/inlineScriptMetadata';
39-
import { CONDA_MANAGER_ID, PYENV_MANAGER_ID, SYSTEM_MANAGER_ID } from '../../common/constants';
40-
import { acquireFileLock, AcquiredFileLock } from '../../common/lockfile.apis';
41-
import { isFileNotFoundError } from '../../common/utils/filesystem';
42-
import { normalizePath } from '../../common/utils/pathUtils';
43-
import { compareReleaseSegments, parseReleaseSegments } from '../../common/utils/pep440Release';
44-
import { getVenvPythonPath } from '../../common/utils/virtualEnvironment';
45-
import { NativePythonFinder } from '../common/nativePythonFinder';
46-
import { createWithProgress, resolveVenvPythonEnvironmentPath } from './venvUtils';
39+
} from '../../../common/inlineScript/metadata';
40+
import { CONDA_MANAGER_ID, PYENV_MANAGER_ID, SYSTEM_MANAGER_ID } from '../../../common/constants';
41+
import { acquireFileLock, AcquiredFileLock } from '../../../common/lockfile.apis';
42+
import { isFileNotFoundError } from '../../../common/utils/filesystem';
43+
import { normalizePath } from '../../../common/utils/pathUtils';
44+
import { compareReleaseSegments, parseReleaseSegments } from '../../../common/utils/pep440Release';
45+
import { getVenvPythonPath } from '../../../common/utils/virtualEnvironment';
46+
import { NativePythonFinder } from '../../common/nativePythonFinder';
47+
import { createWithProgress, resolveVenvPythonEnvironmentPath } from '../venvUtils';
4748

4849
const BASE_INTERPRETER_MANAGER_IDS = new Set([
4950
SYSTEM_MANAGER_ID,
@@ -160,7 +161,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
160161
}
161162
}
162163
} catch (error) {
163-
this.log.error(`Failed to set up inline-script environment: ${this.errorMessage(error)}`);
164+
this.log.error(`Failed to set up inline-script environment: ${getErrorMessage(error)}`);
164165
return undefined;
165166
}
166167
}
@@ -230,7 +231,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
230231
return { environment, canonicalPath: await fs.realpath(executable) };
231232
} catch (error) {
232233
this.log.warn(
233-
`Skipping base interpreter that cannot be resolved at ${executable}: ${this.errorMessage(error)}`,
234+
`Skipping base interpreter that cannot be resolved at ${executable}: ${getErrorMessage(error)}`,
234235
);
235236
}
236237
}
@@ -277,20 +278,20 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
277278
await lock.retain();
278279
} catch (error) {
279280
this.log.error(
280-
`Failed to mark the inline-script cache lock as retained: ${this.errorMessage(error)}`,
281+
`Failed to mark the inline-script cache lock as retained: ${getErrorMessage(error)}`,
281282
);
282283
}
283284
}
284285
return build.environment;
285286
} catch (error) {
286-
this.log.error(`Failed to create or reuse inline-script cache entry: ${this.errorMessage(error)}`);
287+
this.log.error(`Failed to create or reuse inline-script cache entry: ${getErrorMessage(error)}`);
287288
return undefined;
288289
} finally {
289290
if (lock) {
290291
try {
291292
await lock.release();
292293
} catch (error) {
293-
this.log.warn(`Failed to release inline-script cache lock: ${this.errorMessage(error)}`);
294+
this.log.warn(`Failed to release inline-script cache lock: ${getErrorMessage(error)}`);
294295
}
295296
}
296297
}
@@ -315,7 +316,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
315316
try {
316317
resolvedEntry = await resolveCacheEntryPath(cacheRoot, envDir);
317318
} catch (error) {
318-
this.log.warn(`Failed to resolve inline-script cache entry: ${this.errorMessage(error)}`);
319+
this.log.warn(`Failed to resolve inline-script cache entry: ${getErrorMessage(error)}`);
319320
return { kind: 'uncertain' };
320321
}
321322
if (!resolvedEntry) {
@@ -369,7 +370,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
369370
try {
370371
await writeMetaJson(envDir, { ...sidecar, lastUsedAt: new Date().toISOString() });
371372
} catch (error) {
372-
this.log.warn(`Failed to update inline-script cache metadata: ${this.errorMessage(error)}`);
373+
this.log.warn(`Failed to update inline-script cache metadata: ${getErrorMessage(error)}`);
373374
}
374375
return { kind: 'reusable', environment };
375376
}
@@ -394,7 +395,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
394395
false, // trackUvEnvironment
395396
);
396397
} catch (error) {
397-
this.log.error(`Failed to build inline-script environment: ${this.errorMessage(error)}`);
398+
this.log.error(`Failed to build inline-script environment: ${getErrorMessage(error)}`);
398399
await this.removeCacheEntry(envDir);
399400
return {};
400401
}
@@ -429,7 +430,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
429430
lastUsedAt: new Date().toISOString(),
430431
});
431432
} catch (error) {
432-
this.log.error(`Failed to record inline-script cache metadata: ${this.errorMessage(error)}`);
433+
this.log.error(`Failed to record inline-script cache metadata: ${getErrorMessage(error)}`);
433434
await this.removeCacheEntry(envDir);
434435
return {};
435436
}
@@ -442,7 +443,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
442443
await fs.remove(envDir.fsPath);
443444
return true;
444445
} catch (error) {
445-
this.log.error(`Failed to remove incomplete inline-script environment: ${this.errorMessage(error)}`);
446+
this.log.error(`Failed to remove incomplete inline-script environment: ${getErrorMessage(error)}`);
446447
return false;
447448
}
448449
}
@@ -456,10 +457,6 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
456457
return compareReleaseSegments(actualRelease, expectedRelease) === 0;
457458
}
458459

459-
private errorMessage(error: unknown): string {
460-
return error instanceof Error ? error.message : String(error);
461-
}
462-
463460
dispose(): void {
464461
this._onDidChangeEnvironments.dispose();
465462
this._onDidChangeEnvironment.dispose();

src/managers/builtin/inlineScriptMain.ts renamed to src/managers/builtin/inlineScript/main.ts

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,12 @@
22
// Licensed under the MIT License.
33

44
import { Disposable, LogOutputChannel, Uri } from 'vscode';
5-
import { EnvironmentManager, PythonEnvironmentApi } from '../../api';
6-
import { traceInfo, traceVerbose } from '../../common/logging';
7-
import { getPythonApi } from '../../features/pythonApi';
8-
import { isInlineScriptsFeatureEnabled } from '../../helpers';
9-
import { NativePythonFinder } from '../common/nativePythonFinder';
10-
import { InlineScriptEnvManager } from './inlineScriptEnvManager';
5+
import { EnvironmentManager, PythonEnvironmentApi } from '../../../api';
6+
import { traceInfo, traceVerbose } from '../../../common/logging';
7+
import { getPythonApi } from '../../../features/pythonApi';
8+
import { isInlineScriptsFeatureEnabled } from '../../../helpers';
9+
import { NativePythonFinder } from '../../common/nativePythonFinder';
10+
import { InlineScriptEnvManager } from './envManager';
1111

1212
/**
1313
* Register the inline-script env manager when the internal

src/test/common/inlineScriptCacheKey.unit.test.ts renamed to src/test/common/inlineScript/cacheKey.unit.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@ import {
88
computeCacheKey,
99
normalizeDependency,
1010
normalizeInterpreterPath,
11-
} from '../../common/inlineScriptCacheKey';
12-
import * as platformUtils from '../../common/utils/platformUtils';
11+
} from '../../../common/inlineScript/cacheKey';
12+
import * as platformUtils from '../../../common/utils/platformUtils';
1313

1414
suite('inlineScriptCacheKey', () => {
1515
let isWindowsStub: sinon.SinonStub;

0 commit comments

Comments
 (0)