From 15566ce48dace2aac9117f7742c5dfaadfe37483 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Tue, 15 Sep 2026 15:05:47 -0700 Subject: [PATCH] refactor: trim comments in the inline-script quick fix Condenses the comments added in #1788 down to what is not obvious from the code: the deliberate inclusion of reportMissingModuleSource, the reason the routing metadata is reseeded after a save, and the reason the companion extension version check sits outside the try block. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/features/inlineScript/setupCodeAction.ts | 29 ++++--------------- src/features/inlineScript/setupEnvironment.ts | 14 ++------- 2 files changed, 9 insertions(+), 34 deletions(-) diff --git a/src/features/inlineScript/setupCodeAction.ts b/src/features/inlineScript/setupCodeAction.ts index 37275ffa..42af8a21 100644 --- a/src/features/inlineScript/setupCodeAction.ts +++ b/src/features/inlineScript/setupCodeAction.ts @@ -18,17 +18,10 @@ import { getInlineScriptRoutingKey, InlineScriptRoutingRegistry } from '../../co import { InlineScriptStrings } from '../../common/localize'; import { isInlineScriptsFeatureEnabled } from '../../helpers'; -/** - * Diagnostic codes meaning "this import did not resolve", lowercased for comparison. - * - * `reportMissingModuleSource` is included deliberately, unlike in Pylance's own - * `isMissingImportDiagnostic`: a stub without source means the package is not installed, which - * setting the script's environment up fixes. - */ const UNRESOLVED_IMPORT_DIAGNOSTIC_CODES: ReadonlySet = new Set([ // Pyright / Pylance / basedpyright. 'reportmissingimports', - 'reportmissingmodulesource', + 'reportmissingmodulesource', // Stub found but no source: the package is not installed. // Ty. 'unresolved-import', 'possibly-missing-import', @@ -36,7 +29,7 @@ const UNRESOLVED_IMPORT_DIAGNOSTIC_CODES: ReadonlySet = new Set([ 'missing-import', 'missing-source', 'missing-source-for-stubs', - // mypy, via ms-python.mypy-type-checker. + // mypy. 'import-not-found', 'import-untyped', ]); @@ -49,25 +42,16 @@ function normalizeDiagnosticCode(code: Diagnostic['code']): string | undefined { return typeof value === 'string' || typeof value === 'number' ? String(value).toLowerCase() : undefined; } -/** - * Whether `diagnostic` reports an import that could not be resolved. Matches on `code`, never on - * `source`: Pyrefly-backed Pylance reports its source as the literal string `pylance + pyrefly`. - */ +/** Whether `diagnostic` reports an unresolved import. Matches on `code` only, never on `source`. */ export function isUnresolvedImportDiagnostic(diagnostic: Diagnostic): boolean { const code = normalizeDiagnosticCode(diagnostic.code); return code !== undefined && UNRESOLVED_IMPORT_DIAGNOSTIC_CODES.has(code); } /** - * Offers "Set up this script's Python environment" as a quick fix on an unresolved import in a `.py` - * file that declares a PEP 723 `# /// script` block and has no inline-script environment yet. - * - * Complements the CodeLens, which is hidden while the document is dirty — the moment a user has just - * typed the import that does not resolve. This provider parses the in-memory buffer instead. - * - * `diagnostics` and `isPreferred` are both left unset: setup installs the block's declared - * dependencies verbatim and may not resolve the import at all, so the action must not claim to fix - * the diagnostic or pre-empt a real import fix. + * Offers inline-script environment setup as a quick fix on an unresolved import, complementing the + * CodeLens that is hidden while the document is dirty. `diagnostics` and `isPreferred` stay unset: + * setup installs the block's declared dependencies verbatim and may not resolve the import at all. */ export class InlineScriptSetupCodeActionProvider implements CodeActionProvider { constructor( @@ -75,7 +59,6 @@ export class InlineScriptSetupCodeActionProvider implements CodeActionProvider { private readonly setupCommand: string, ) {} - /** Gates run cheapest-first, and before any parsing: VS Code may call this on every cursor move. */ public provideCodeActions( document: TextDocument, _range: Range, diff --git a/src/features/inlineScript/setupEnvironment.ts b/src/features/inlineScript/setupEnvironment.ts index 488893fc..99ba8ef7 100644 --- a/src/features/inlineScript/setupEnvironment.ts +++ b/src/features/inlineScript/setupEnvironment.ts @@ -97,7 +97,6 @@ async function seedRoutingMetadataForClosedScript(scriptUri: Uri, routing: Inlin } } -/** The open text document backing `scriptUri`, if the user has it open. */ function findOpenDocument(scriptUri: Uri): TextDocument | undefined { const scriptPath = normalizePath(scriptUri.fsPath); return getOpenTextDocuments().find( @@ -105,11 +104,6 @@ function findOpenDocument(scriptUri: Uri): TextDocument | undefined { ); } -/** - * Save `scriptUri` if it is open with unsaved changes, so setup reads what the user actually sees. - * - * Returns `false` when the document could not be saved; setup must not run in that case. - */ async function saveScriptBeforeSetup(scriptUri: Uri, routing: InlineScriptRoutingRegistry): Promise { const document = findOpenDocument(scriptUri); if (!document?.isDirty) { @@ -120,9 +114,8 @@ async function saveScriptBeforeSetup(scriptUri: Uri, routing: InlineScriptRoutin return false; } traceVerbose(`Saved ${scriptUri.fsPath} before setting up its inline-script environment.`); - // Seeding here is load-bearing: without it a just-typed block goes from no metadata to an - // identity while `create` runs, which `setUpInlineScriptEnvironment` reads as a concurrent edit - // and silently skips the association. + // Load-bearing: without it a just-typed block gains its identity mid-`create`, which reads as a + // concurrent edit and silently skips the association. const metadata = await readInlineScriptMetadataFromFile(scriptUri); if (metadata) { routing.setMetadata(scriptUri, metadata); @@ -164,8 +157,7 @@ export function setupInlineScriptEnvironmentHandler( notifyInlineScriptSetupOutcome(uri, routing); return; } - // Kept out of the try: the environment is already set up, so a failure in this follow-up - // must not be reported to the user as a setup failure. + // Outside the try: the environment is already set up, so a failure here is not a setup failure. await promptUpdateExtensionsForInlineScripts().catch((error) => traceError('Failed to check companion extension versions for inline scripts:', error), );